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

Fixing dependencies #2547

Merged
merged 13 commits into from Nov 20, 2016

Conversation

@felixfontein
Copy link
Contributor

felixfontein commented Oct 25, 2016

Implementation of #2536.

The first part ensures that the .dep file is always written and specified as a second target of the render_post task, so that the .dep file will be created if it isn't there yet. (Otherwise changing dependencies of the post might be ignored when the .dep file is gone.)

…get for render_posts plugin.
@Kwpolska

This comment has been minimized.

Copy link
Member

Kwpolska commented Oct 26, 2016

I’d rather not create empty .dep files. Are there any issues with the current approach?

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

felixfontein commented Oct 26, 2016

Yes: if the .dep file was removed for some reasons, and a file which should have been mentioned there was modified, the post won't be recompiled because Nikola cannot distinguish between an empty .dep file and the .dep file not being there.

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

felixfontein commented Oct 28, 2016

How about letting the compiler decide whether .dep must be created (and let it insert the .dep target manually via the new function)?

@Kwpolska

This comment has been minimized.

Copy link
Member

Kwpolska commented Oct 29, 2016

Are compilers the only things that can provide .dep files? If yes, we could do that.

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

felixfontein commented Oct 29, 2016

Well, of course any other plugin could also create .dep files for whatever it wants :) To my knowledge, .dep files are only generated by compilers. Or more precisely, nowadays they are created by the Post object.

A problem is that sometimes compilers also create .dep files with a different meaning; for example the WordPress compiler. So always writing .dep files is probably not a good idea at all. How about: the plugin can indicate that the Post object should always write the .dep file, and add it to the list of targets. This will then be used by the ReST plugin and which else of the standard plugins uses the .dep mechanism.

I'll try that out.

… be always created and added as a target.
@felixfontein

This comment has been minimized.

Copy link
Contributor Author

felixfontein commented Oct 29, 2016

Ah. There's actually a bug in the .dep system: .dep files are written for different languages, but only the .dep file for the default language is ever read.

@@ -250,6 +250,7 @@ class PageCompiler(BasePlugin):
friendly_name = ''
demote_headers = False
supports_onefile = True
use_dep_file = False # If set to true, the .dep file is always written and added as a target

This comment has been minimized.

Copy link
@ralsina

ralsina Nov 20, 2016

Member

Is there a case when we want this to be False? If not, then let's lose the option and simplify.
Other than that, LGTM.

This comment has been minimized.

Copy link
@felixfontein

felixfontein Nov 20, 2016

Author Contributor

Well, some compilers might use .dep files for something else. Actually, the WordPress compiler does: it uses the .dep file to store a JSON-encoded file which not only contains file dependencies, but potentially other dependencies. (I added that feature when .dep files were completely handled by the compilers themselves.) I don't know if any other plugin not shipped with Nikola directly uses .dep like this as well.

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Nov 20, 2016

Member

Or perhaps we could make it so .dep always means file dependencies separated by newlines, and the WordPress compiler creates its custom .wpdep format?

And I think the default should be True.

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

felixfontein commented Nov 20, 2016

Hmm, there seem to be a LOT of failing stuff in the tests which are not caused by this PR. Like all the flake8 warnings. I was only able to get these warnings after upgrading flake8. Also, it seems like the baseline is (again) out-of-date, as the generated thumbnails differ.

felixfontein added a commit to getnikola/plugins that referenced this pull request Nov 20, 2016
@@ -250,6 +250,7 @@ class PageCompiler(BasePlugin):
friendly_name = ''
demote_headers = False
supports_onefile = True
use_dep_file = False # If set to true, the .dep file is always written and added as a target

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Nov 20, 2016

Member

Or perhaps we could make it so .dep always means file dependencies separated by newlines, and the WordPress compiler creates its custom .wpdep format?

And I think the default should be True.

@@ -273,7 +278,18 @@ def _read_extra_deps(self, post):

def register_extra_dependencies(self, post):
"""Add dependency to post object to check .dep file."""
post.add_dependency(lambda: self._read_extra_deps(post), 'fragment')
def create_lambda(lang):

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Nov 20, 2016

Member

Add a comment for future readers to explain this:

# We create a lambda like this so we can pass `lang` to it, because if we didn’t add that function, `lang` would always be the last language in TRANSLATIONS.

This comment has been minimized.

Copy link
@felixfontein

felixfontein Nov 20, 2016

Author Contributor

Fixed that.

This comment has been minimized.

Copy link
@felixfontein

felixfontein Nov 20, 2016

Author Contributor

@.dep file name: Yes, we could do that, too. I'll change it.

felixfontein and others added 4 commits Nov 20, 2016
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@felixfontein

This comment has been minimized.

Copy link
Contributor Author

felixfontein commented Nov 20, 2016

I'll wait until the checks are done, and then I'll merge.

@felixfontein felixfontein merged commit d6dde70 into master Nov 20, 2016
4 checks passed
4 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@felixfontein felixfontein deleted the fixing-post-dependencies branch Nov 20, 2016
Kwpolska added a commit to getnikola/plugins that referenced this pull request Nov 20, 2016
Fixing .dep filename, and adjusting to getnikola/nikola#2547.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.