-
Notifications
You must be signed in to change notification settings - Fork 567
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
Fixed template assignments with old basic and full names with tpl extension #1387
Conversation
if os.path.exists(conf_file): | ||
with open(conf_file) as f: | ||
conf = recursive_update(json.load(f), conf) | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor optimization, this does change the algorithm to find the first instead of the last match. I think this is fine but I'd like other eyes on it in case I need to reverse the iteration ordering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not 100% sure about the break
statement.
What if configuration is overriden in a higher precedence extra template dir?
Ping @maartenbreddels. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it will not merge the configuration with other matches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't before because it overwrote the conf
and conf_file
whenever it found a template until the last one found. I don't think it changes the behavior beyond preferring the first exact match over the last exact match. If you're really worried I can remove the optimization.
adb1ba2
to
56f0e08
Compare
thanks for the ping. i will look into this Monday morning paris time. |
56f0e08
to
d5a22d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @MSeal. Will leave for @SylvainCorlay to review and comment/merge.
Hmm before merging the comment thread on #1386 should be read. I think we (I?) eliminated the |
if os.path.exists(conf_file): | ||
with open(conf_file) as f: | ||
conf = recursive_update(json.load(f), conf) | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Not sure the recursion on root_dirs should be conditional on found_at_least_one
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recursion isn't conditional, it recurses to completion before returning back to this context
template_names.extend(['classic', 'base']) | ||
if base_template == 'full': | ||
template_names.extend(['lab', 'base']) | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach may collide with an actual template called full
or basic
.
Maybe we should condition on this at a higher level...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of changing the include paths inside template files to include the template name. I think that would resolve a few issues and not require these hacks for the backwards compatibility, as template extensions are similarly running into 2nd hop extension path failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated PR with the change I discussed -- extending existing templates from custom templates no longer explodes when files are included across template boundaries. e.g. Fixes #1394
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing the include paths inside template files to include the template name
I am not sure about this, because it is a backward incompatible change with respect to the current behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is it backwards incompatible? The templates behave identically within nbconvert but now when extended from external templates will behave correctly (since they don't have the proper template paths set to include this template). It also protects from multiple inheritance of file extensions -- e.g. there's ambiguity when importing null.js is some places if you didn't include the template name in the extension path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is backward incompatible for custom template authors who inherit from base.
d5a22d6
to
00cf1d2
Compare
I added the basic template back as well, since it has a need and we made using it more difficult and broke backwards compatibility. |
@MSeal it should not be required to add the path to the base template in jinja templates, unless you override them locally. Does your change make it necessary? |
@maartenbreddels @MSeal would you like to have a synchronous discussion about how to fix this the best early next week? |
Only in the sense that the backwards compatibility templates were not working without it. Similarly there's several open issues pointing out that extending
Would have to be tomorrow (Sunday) or Tuesday morning as my Monday has no wiggle room on my end. |
Tuesday works for me. I think we should not prefix with Going over the code, and trying to make the test pass without the prefixes, I don't think we can go with full 5.x compatibility as it is now. I think if we want compatibility with full.tpl and base.tpl, we should just copy the old content of nbconvert 5.x, and keep/add the deprecation warning. |
Invite sent.
I see. Unfortunately since the template_paths don't dynamically reflect extensions made, supporting that pattern breaks the ability for folks to extend the |
Closing as alternative fixes were merged |
In the refactor that simplified the template_name fetching we broke using
template=basic
despite the path to the compatibility templates being added to the search paths. This fixes this by searching the compatibility directory last.Second half of resolving #1384