- Notifications
You must be signed in to change notification settings - Fork782
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fix reload dir behavior#2598
base:master
Are you sure you want to change the base?
Conversation
| if Path.cwd() not in directory.parents: | ||
| self.reload_dirs.append(directory) | ||
| if Path.cwd() not in self.reload_dirs: | ||
| self.reload_dirs.append(Path.cwd()) |
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 in order to correctly match the documentation you still need to add the cwd when config.reload_dirs is empty, so the code that you removed can be replaced with:
ifnotconfig.reload_dirs:self.reload_dirs.append(Path.cwd())
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.
That's what I had there earlier, but it seems thatPath.cwd() is added somewhere else in the chain, this function never actually receives emptyconfig.reload_dirs, which is why I wasn't able to test this in any sensible way.
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.
You're right, it's probably just added when initializing the config in that case. As a minor thing, I would still change this code a little bit to usePath.cwd() instead ofPath(os.getcwd()) so that it's also easier to search where that is added in the chain.
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.
Sure, I wanted to keep the change minimal and isolated, but changingPath(os.getcwd()) toPath.cwd() won't hurt anything, so I applied the suggestion.
adhami3310 commentedMar 21, 2025
i tried doing so in#2583 but it seems the maintainer has better things to do :/ since then i moved to a different library that handled reload dir in a sensible way |
Pretty rude considering your company leverages the contributions I've made to the ecosystem. |
adhami3310 commentedMar 21, 2025
Everyone appreciates your contributions! We're also trying to contribute back :) If you're busy with other things, that's ok. Everyone's been there, I've been there. I think, however, one shouldaim to communicate with their community on the timescale they expect things to happen. If the community finds an issue in the project they want to fix, and they offer the fix to you, it's (I'm not going to say common) decency to acknowledge them. In the same way you expect people to appreciate your contributions make them feel like their contributions are appreciated. I talk in ideas of course, who among us is perfect. |
Summary
This PR fixes the behavior of
--reload-dirto match the documentation. The documentation states:also:
However, the current implementation always adds
Path.cwd()to list of watched directories. This is undesirable and contradicts the documentation.Path.cwd()should only be added as a default when no--reload-diris specified.Checklist