-
Notifications
You must be signed in to change notification settings - Fork 122
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
Allow towncrier
to traverse back up directories looking for the configuration file
#601
Conversation
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.
Many thanks for the changes.
They look good.
I left a few minor inline comments.
The major comment is about the missing test for the case in which the config file was not found, even when traversing the whole tree.
I think that we should have such a test before merging this.
Thanks again!
src/towncrier/_settings/load.py
Outdated
@@ -67,7 +67,7 @@ def load_config_from_options( | |||
) -> tuple[str, Config]: | |||
if config_path is None: | |||
if directory is None: | |||
directory = os.getcwd() | |||
return traverse_for_config(None) |
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 that this should be private.
I know that this is from _settings.load
, but I think that we should make it explicit that we don't want to expose this as a public API.
Also, there are a lot of branches here,
so I think that it would help to add an explicit comment for this branch.
With the missing docstring for the load_config_from_options
it's a bit hard to understand the purpose of this function.
Can we have something like this.
It lools like a bit of duplication between
traverse_for_config
and these following lines
base_directory = os.path.abspath(directory)
config = load_config(base_directory)
maybe have something like this
if config_path is None:
return _traverse_for_config(directory)
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 don't think there's any assumption that non underscored methods become automatically part of a public API is there? If people want to shoot themselves in the foot, let em 🤠
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've added a docstring and some comments to explain the different branches (I don't like the complexity of the towncrier's loading, but I'm not going to rewrite it just for this addition)
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.
Actually, I see what you're getting at with the branching logic now. I've simplified it (and added another test).
…ts of the different branch 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.
Thanks for the updates.
All good.
Thanks for the cleanup for |
Description
Fixes #433
Checklist
src/towncrier/newsfragments/
. Describe yourchange and include important information. Your change will be included in the public release notes.
docs/tutorial.rst
is still up-to-date.