Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactoring of jbake configuration #341

Merged
merged 19 commits into from
Aug 14, 2018

Conversation

ancho
Copy link
Member

@ancho ancho commented Feb 9, 2017

I started to refactor the configuration of jbake and it's components. See #340.

In general it's finished. But I want to write some more tests and add some logging in case options could not be found and things like that.

There are only two classes left with a direct dependency to a CompositeConfiguration, which are configuration specific classes.

Please take your time to review the changes. I hope you like it.

@coveralls
Copy link

Coverage Status

Coverage increased (+9.8%) to 82.718% when pulling f0db5bd on ancho:feature/configuration-refactoring into 0f7b6c6 on jbake-org:master.

@coveralls
Copy link

coveralls commented Feb 12, 2017

Coverage Status

Coverage increased (+10.08%) to 82.995% when pulling 45199f9 on ancho:feature/configuration-refactoring into 0f7b6c6 on jbake-org:master.

@ancho
Copy link
Member Author

ancho commented Feb 12, 2017

Ok. I think I'm finished.

@ancho ancho changed the title [WIP] refactoring of jbake configuration refactoring of jbake configuration Feb 12, 2017
@ancho
Copy link
Member Author

ancho commented Mar 11, 2017

Hello? Anybody out there? :)

@jonbullock
Copy link
Member

Still here.... just busy preparing for a talk at the moment :)

@ancho
Copy link
Member Author

ancho commented Mar 14, 2017

All right. Just a ping. Worked obviously :)
Good luck with the talk then.

@jonbullock
Copy link
Member

I haven't forgotten about this, I'll try and catch you on IRC to discuss what order is best to merge this and the other major PR's (such as the switch to Gradle).

@jonbullock jonbullock added this to the v3.0.0 milestone Apr 17, 2017
@ancho ancho force-pushed the feature/configuration-refactoring branch from 45199f9 to a788ee1 Compare May 9, 2017 17:38
@ancho ancho force-pushed the feature/configuration-refactoring branch from a788ee1 to 3d55f33 Compare May 24, 2017 07:21
@coveralls
Copy link

coveralls commented May 24, 2017

Coverage Status

Coverage increased (+1.1%) to 81.211% when pulling 2486ab3 on ancho:feature/configuration-refactoring into 5a368e8 on jbake-org:master.

@ancho
Copy link
Member Author

ancho commented May 24, 2017

Hmm...I'm rebasing from time to time and I ask my self if it would find a way earlier into master if I add deprecated versions of the old Method/Constructor signatures that changed during the refactoring?

@jonbullock
Copy link
Member

Thanks for rebasing this and keeping it up to date with master 👍

I've scheduled in a 2.7.0 release now so if you can add those deprecated methods/constructors for the build plugins (anything else as well?) I don't see why we can't include this in that release. What do you think?

@ancho
Copy link
Member Author

ancho commented May 31, 2017

I'll try to keep it up to date. It would be nice to see this as soon as possible. But I'm not sure if it's possible to get a 100% compatible version. The FileUtil lost the public static findExtension for example. But it doesn't make sense to add this method there. It's the wrong place and is part of the DefaultJBakeConfiguration now.

I'll open a new branch for this experiment and concentrate on the main classes like

  • Crawler
  • Parser
  • Asset
  • Oven
  • ContentStore
  • Renderer
  • implementations of MarkupEngine
  • implementations of RenderingTool
  • etc.

I touched 69 files. Ok...I can substract the tests but that will take some time.
The question is what classes are API classes and which modifications count as a normal refactoring changes that don't violate semantic versioning.

@ancho
Copy link
Member Author

ancho commented Jun 5, 2017

Ok. I created a new branch with restored methods and constructors.

I tested the snapshot version against the gradle plugin and my website project and it still works.

The CI failed a few times before, because the singleton ModelExtractors had registered engines from previous tests. So I had to implement a reset method to keep the tests clean and seperated from each other.
The same applies for DocumentTypes which already had a reset method.

I'm not quite sure if this is a problem for production too...

@jonbullock
Copy link
Member

I can test the branch against the Maven plugin as I've just published v0.2.0 if you want to see if there is any major problems? If not we can schedule it in for 2.7.0.

@ancho
Copy link
Member Author

ancho commented Jun 6, 2017

That would be great. I will try to implement a test to verify we do not get problems with the singletons running bake in a loop removing document types. The only scenario I can think of at the moment.

@ancho ancho force-pushed the feature/configuration-refactoring branch from 3d55f33 to 2e2b580 Compare March 10, 2018 09:56
@ancho
Copy link
Member Author

ancho commented Mar 10, 2018

Rebased to master and squashed to make a review easier.

@ancho ancho modified the milestones: v3.0.0, v2.7.0 Mar 10, 2018
@ancho ancho force-pushed the feature/configuration-refactoring branch from 2e2b580 to 55ac964 Compare March 10, 2018 10:33

private void displayLegacyConfigFileWarningIfRequired() {
LOGGER.warn("You have defined a part of your JBake configuration in {}", LEGACY_CONFIG_FILE);
LOGGER.warn("Usage of this file is being deprecated, please rename this file to: {} to remove this warning", CONFIG_FILE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we say that Usage of this file is deprecated and will be removed in JBake 3.0. Please rename this file to {}?

Date date = df.parse((String) value);
contents.put("date", date);
} catch (ParseException e) {
LOGGER.error("Unable to parse revdate. Expected {}", dateFormat, e);
}
}
if (key.equals("jbake-tags")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we extract "jbake-tags" to a constant?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. There is a lot of stuff we should put into constants or find better substitutes.
I just wanted to focus on the configuration part with this PR.

@ancho ancho force-pushed the feature/configuration-refactoring branch from 189cbc3 to 301af6a Compare April 9, 2018 08:51
@ancho ancho force-pushed the feature/configuration-refactoring branch from 931d902 to 2486ab3 Compare August 4, 2018 09:02
@jonbullock
Copy link
Member

Shall we merge this before we merge smaller PR's like #471 ?

@ancho
Copy link
Member Author

ancho commented Aug 10, 2018

Makes sense. That way we "just" need to adjust the outstanding PR's.

@jonbullock jonbullock modified the milestones: v2.7.0, v2.6.2 Aug 12, 2018
@jonbullock
Copy link
Member

Do you want to do the honours for this one?

@ancho
Copy link
Member Author

ancho commented Aug 13, 2018

My pleasure! Do you need to prepare something before? Should I wait or just merge it right now?

@jonbullock
Copy link
Member

jonbullock commented Aug 13, 2018 via email

@ancho ancho merged commit 4f3a18a into jbake-org:master Aug 14, 2018
ancho added a commit to ancho/jbake that referenced this pull request Mar 29, 2020
a fragment of an not properly removed file during refactoring jbake-org#341

closes jbake-org#608
@ancho ancho mentioned this pull request Mar 29, 2020
@ancho ancho deleted the feature/configuration-refactoring branch May 1, 2020 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants