-
Notifications
You must be signed in to change notification settings - Fork 19
fix(init): exit parent template dir context after using it #795
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
base: main
Are you sure you want to change the base?
fix(init): exit parent template dir context after using it #795
Conversation
bepri
left a comment
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.
Looks good, thanks for updating the tests too. Just one nit :)
craft_application/commands/init.py
Outdated
|
|
||
| @property | ||
| def parent_template_dir(self) -> pathlib.Path: | ||
| def parent_template_dir(self) -> AbstractContextManager[pathlib.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.
| def parent_template_dir(self) -> AbstractContextManager[pathlib.Path]: | |
| @contextlib.contextmanager | |
| def parent_template_dir(self) -> pathlib.Path | |
| yield importlib.resources.path(self._app.name, "templates") |
It's not letting me highlight the whole block for this suggestion (grr) so sorry if it looks weird. I would wrap this using a decorator instead, it's a more common pattern with our tools.
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.
Under this example wouldn't it need to be something like:
with importlib.resources.path(...) as p:
yield pOtherwise you'd have a context manager that yields a context manager.
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.
facepalm, yeah, that's correct
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 return type isn't right, do you have any preferences between Iterator[pathlib.Path], Generator[pathlib.Path] or Iterable[pathlib.Path]?
I'll probably revisit this next week to add the back-compatibility.
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 think Generator makes the most sense to me for something that's meant to be used as a context manager, but I don't have a strong preference
craft_application/commands/init.py
Outdated
|
|
||
| @property | ||
| def parent_template_dir(self) -> pathlib.Path: | ||
| def parent_template_dir(self) -> AbstractContextManager[pathlib.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.
This is a breaking change, since a child class could override this. We might need to instead deprecate this method and add a new one with the right behaviour.
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.
Ok I've attempted it. Probably not 100% compatible since the subclass could override __getattribute__ or something weird like that.
medubelko
left a comment
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.
Just a small suggestion for the changelog.
1b850e6 to
265bd1f
Compare
265bd1f to
a724ea6
Compare
The directory isn't guaranteed to exist after the context manager exits.
a724ea6 to
8d6abd3
Compare
The directory isn't guaranteed to exist after the context manager exits.
make lint && make test?docs/reference/changelog.rst)?There were some unrelated lint/test failures.