Skip to content
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

🐛 Fixed --reload-dir option not working as expected. #2107

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bybatkhuu
Copy link

@bybatkhuu bybatkhuu commented Sep 21, 2023

--reload-dir specified, but default dir(cwd) still been watching

Not working as cli help explained:

txt

uvicorn --help:
--reload-dir PATH   Set reload directories explicitly, instead
                    of using the current working directory.

So, I tried to fix it by counting self.reload_dirs, and if it's not provided by arguments, then it will add the default current directory as the watching directory.

Previous discussion: #1833

And also: #1326

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@iudeen
Copy link
Contributor

iudeen commented Sep 22, 2023

This solution looks fine for me on a quick glance. I'll see if I can review this properly before giving my verdict. Also, Fix the tests please :)

@imdoroshenko
Copy link

imdoroshenko commented Dec 6, 2023

This issue can block the usage of the reload feature in some cases. I work on a CLI tool that should start the server from a host directory where a lot FS operations happen every few seconds. In my case, reload-dir targets the correct directory with the server source code, but those lines cause constant reload because Path.cwd() is added to the reload_dirs no matter what.

edit:
This issue is also hard to detect because logs print directories it plans to watch before it adds Path.cwd() there.

INFO:     Will watch for changes in these directories: ['directories form --reload-dir']

@akefirad
Copy link

akefirad commented Jan 2, 2024

This is a bit suboptimal DX. Any help needed with this fix? I can fix the tests if @bybatkhuu doesn't have time. LMK.

@bybatkhuu
Copy link
Author

Sorry, I don't have time to understand tests and fix for this case. @akefirad, of course. Thank you~

@loukkyy
Copy link

loukkyy commented Oct 21, 2024

anyone working on this? Would be good to add this fix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants