Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Things I noticed while going through the code:
Node.fieldsandNode.attributescould be dataclasses / annotations instead of a tuple of strings. I already added annotations for all the names to each class, so that type checking the parser and compiler was useful. No nodes add extraattributes.copymethod which usesobject.__new__and other "magic" to create a copy. This code seems to be from 2008, when Python'scopymodule might not have been available? In most cases it should be replaceable with a call tocopy.copy(), except many of the classes also do__copy__ = copy.indent,enter_frame,push_parameter_definitions,push_context_reference,push_assign_tracking,_output_child_pre.LRUCachein favor offunctools.lru_cache. I replacedget_spontaneous_environment, but the other uses are a bit trickier.environment.overlayis not used anywhere. It seems it was intended to be used with extensions, but the documentation about it is unclear and there are no examples of its use.environment.handle_exceptionshould be deprecated. It says it can be used to "return a value or raise a rewritten exception", but it's only used for the later and there are no examples of overriding it. It's called by various functions that have different return types, as well as in some places where returning instead of raising would be ignored and silence fatal errors. So overriding it wouldn't work as expected.compiler.generatefunction is not public, is only called fromEnvironment._generate(confusingly named), and has an inconvenient return signature. If a file object is passed in (never used or documented), it doesn't return anything, but it seems it would only be useful for it to return a string. Could either remove stream argument, or make it two separate functions or class methods.__init_subclass__. Only one left isNodeType, which should probably be reexamined anyway.async_variantdecorator (internal only) needs work, I tried to type it but caused mypy to crash. It's generally messy and I already had to reconfigure it a bit when removing the async patching.runtime.Macrocan be called from Python code withtemplate.module, in which case it won't have an eval context to look forautoescape, so it falls back toenvironment.autoescape. However, it wasn't accounting for the fact that that could be a callable.Checklist:
CHANGES.rstsummarizing the change and linking to the issue... versionchanged::entries in any relevant code docs.pre-commithooks and fix any issues.pytestandtox, no tests failed.