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

Attempting to fix #2496. #2695

Merged
merged 12 commits into from Apr 2, 2017
Merged

Attempting to fix #2496. #2695

merged 12 commits into from Apr 2, 2017

Conversation

@felixfontein
Copy link
Contributor

felixfontein commented Mar 12, 2017

fixes #2496. closes #2498 (superseeded by this).

@felixfontein
Copy link
Contributor Author

felixfontein commented Mar 12, 2017

The last commit should fix the texts, but also induces a potentially backwards compatibility breaking change in case a post scanner plugin needs the final site._GLOBAL_CONTEXT in its set_site.

The only way to avoid this is probably to get rid of the bad compilers optimization completely. This would also make all the new code superfluous and simplify logic a lot.

@felixfontein
Copy link
Contributor Author

felixfontein commented Mar 14, 2017

@ralsina: any thoughts and comments?

@felixfontein felixfontein force-pushed the fix-2496-second-try branch from 1d1712d to 2120fa5 Mar 26, 2017
@felixfontein felixfontein requested a review from Kwpolska Mar 26, 2017
@Kwpolska
Copy link
Member

Kwpolska commented Mar 28, 2017

@felixfontein I might not have enough time for a review until the weekend.


def supported_extensions(self):
"""Return a list of supported file extensions, or None if such a list isn't known beforehand."""
return list(set([os.path.splitext(x[0])[1] for x in self.site.config['post_pages']]))

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Mar 28, 2017

Member

This can be made simpler. Since we dropped 2.6 support, you can use set comprehensions ({i for i in x}). And even if we didn’t, using a generator expression inside set(i for i in x) would look better.

        return list({os.path.splitext(x[0])[1] for x in self.site.config['post_pages']})

This comment has been minimized.

Copy link
@felixfontein

felixfontein Mar 28, 2017

Author Contributor

Fixed.

@@ -1729,7 +1759,7 @@ def generic_rss_feed(self, lang, title, link, description, timeline,
post.date.astimezone(dateutil.tz.tzutc())),
'categories': post._tags.get(lang, []),
'creator': post.author(lang),
'guid': post.guid(lang),
'guid': post.permalink(lang, absolute=True),

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Mar 28, 2017

Member

Was this intentional or are those just some leftovers?

This comment has been minimized.

Copy link
@felixfontein

felixfontein Mar 28, 2017

Author Contributor

These are definitely leftovers!

This comment has been minimized.

Copy link
@felixfontein

felixfontein Mar 28, 2017

Author Contributor

Looks like that was the only one which got in. I've removed it by now.

@felixfontein
Copy link
Contributor Author

felixfontein commented Mar 28, 2017

@Kwpolska: take your time! A couple of days more don't really matter ;)

self.disabled_compiler_extensions = defaultdict(list)
self.bad_compilers = set([])
for k, v in compilers.items():
# self.config['COMPILERS'][k] = sorted(list(v))

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Apr 1, 2017

Member

This requires a comment to explain (and shouldn’t have commented-out code)

This comment has been minimized.

Copy link
@felixfontein

felixfontein Apr 1, 2017

Author Contributor

Looks like a leftover from the original code. I've removed it and simplified the thing.

utils.LOGGER.debug('Not loading compiler extension {}', p[-1].name)
if to_add:
self.plugin_manager._candidates = self._filter_duplicate_plugins(to_add)
self.plugin_manager.loadPlugins()

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Apr 1, 2017

Member

Are plugins loaded through here activated?

This comment has been minimized.

Copy link
@felixfontein

felixfontein Apr 1, 2017

Author Contributor

Yes. Until that point, only plugins of type PostScanner have been activated, and all others not. Since we only added plugins of type PageCompiler or compiler extensions (plugins having compiler set) no plugin should be forgotten.

(Except if a post scanner plugin has compiler set in its .plugin file. I assumed such plugins don't exist.)

compilers[compiler].add(candidate)

# Avoid redundant compilers
# Remove compilers that match nothing in POSTS/PAGES

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Apr 2, 2017

Member

Could use an explanation of how we gather the compilers, and perhaps a better name than bad_compilers.

This comment has been minimized.

Copy link
@felixfontein

felixfontein Apr 2, 2017

Author Contributor

I removed bad_compilers, since the same information is already contained in disabled_compilers and nothing in core or plugins was using it.

Is the explanation in 53e5f6e good enough, or do you want something more detailed?

@Kwpolska

This comment has been minimized.

Copy link
Member

Kwpolska commented on nikola/plugins/command/new_post.py in 53e5f6e Apr 2, 2017

Too verbose, and emphasises the wrong part. Post scanners are relevant only to a few people, while POSTS/PAGES apply to everyone.

! not in the POSTS/PAGES tuples and any post scanners (unused)

This comment has been minimized.

Copy link
Contributor Author

felixfontein replied Apr 2, 2017

Fixed.

@Kwpolska
Copy link
Member

Kwpolska commented Apr 2, 2017

LGTM. How much testing have you done? Have you tested it with some other PostScanner plugin?

@felixfontein
Copy link
Contributor Author

felixfontein commented Apr 2, 2017

I've tested it with my pages and blogs; they all use the hierarchical pages post scanner plugin (https://plugins.getnikola.com/v7/hierarchical_pages/), and one of them another post scanner plugin I'm currently working on. So far it seems to work fine.

@felixfontein
Copy link
Contributor Author

felixfontein commented Apr 2, 2017

(I'm using a slightly adjusted version of the hierarchical pages plugin which defines supported_extensions; and used the other plugin first without and later with such a function as well. So it looks like it works both for non-updated as well as updated plugins.)

Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska
Copy link
Member

Kwpolska commented Apr 2, 2017

It works fine in both cases (tested with the plugins site). Merging.

@Kwpolska Kwpolska merged commit 4fb64ab into master Apr 2, 2017
1 of 5 checks passed
1 of 5 checks passed
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
codacy/pr Good work! A positive pull request.
Details
@Kwpolska Kwpolska deleted the fix-2496-second-try branch Apr 2, 2017
felixfontein added a commit to getnikola/plugins that referenced this pull request Apr 2, 2017
felixfontein added a commit to getnikola/plugins that referenced this pull request Apr 2, 2017
@felixfontein
Copy link
Contributor Author

felixfontein commented Apr 2, 2017

Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.