Don't include cwd() when non-empty --reload-dirs is passed
#2598
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.
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:
if not config.reload_dirs:
self.reload_dirs.append(Path.cwd())There was a problem hiding this comment.
That's what I had there earlier, but it seems that Path.cwd() is added somewhere else in the chain, this function never actually receives empty config.reload_dirs, which is why I wasn't able to test this in any sensible way.
There was a problem hiding this comment.
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 use Path.cwd() instead of Path(os.getcwd()) so that it's also easier to search where that is added in the chain.
There was a problem hiding this comment.
Sure, I wanted to keep the change minimal and isolated, but changing Path(os.getcwd()) to Path.cwd() won't hurt anything, so I applied the suggestion.
|
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. |
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 should aim 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. |
cwd() when non-empty --reload-dirs is passed
|
Hmmm... What about the scenario when you run If you change the I can see how confusing it can be for people that, for example, just include a folder with jinja files into |
If it were up to me, I wouldn't automagically add anything to the watchlist. If you specify |
|
One more point is that I tried to make this PR as small as possible and not introduce any behavior changes apart from fixing the implementation to match the docs. I can see some arguments for adding the application dir (as well as some arguments against it), but I think that adding the application dir should be out of scope of this particular PR. |
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