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

add note in Settings on using WatchFiles on WSL #1647

Merged
merged 5 commits into from Sep 14, 2022

Conversation

danroozemond
Copy link
Contributor

@danroozemond danroozemond commented Sep 14, 2022

Following a frustrating 2hours of debugging why reloading stopped working when I rebuilt my Docker contrainer with uvicorn recently, stumbling upon @samuelcolvin 's very helpful comment on WATCHFILES_FORCE_POLLING, and @Kludex 's subtle hint that PR's are welcome to improve the documentation.... here's a PR that improves the documentation.

Relates to the discussion here: #1437

Hope it's helpful!

Copy link
Sponsor Contributor

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise LGTM.

docs/settings.md Outdated Show resolved Hide resolved
@samuelcolvin
Copy link
Sponsor Contributor

Just to be clear, WATCHFILES_FORCE_POLLING doesn't have to have any special value, it just needs to be set, set the source.

I guess we could interpret empty WATCHFILES_FORCE_POLLING as not set, but that's a pretty special case.

danroozemond and others added 2 commits September 14, 2022 16:29
Adopt suggested text change from PR#1647

Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
@danroozemond
Copy link
Contributor Author

Thanks for the review & suggestions, @samuelcolvin , I've adopted your changed and added a link to the watchfiles documentation.

@samuelcolvin
Copy link
Sponsor Contributor

Great, thanks for this.

docs/settings.md Show resolved Hide resolved
@Kludex
Copy link
Sponsor Member

Kludex commented Sep 14, 2022

you might have to set the environment variable WATCHFILES_FORCE_POLLING

Is it a "might" or you "need" to set the env variable?

@samuelcolvin
Copy link
Sponsor Contributor

Well, it depends on the environment, in some (e.g. docker on WSL) no changes are detected via the default file system watcher, but I don't have an finite list of environments where it does and doesn't work, hence I suggested "might" to put the decision in the hands of the developer, but up to you - I can see it either way.

Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @danroozemond 🙏

Thanks @samuelcolvin for reviewing it 👍

@Kludex Kludex merged commit 591462d into encode:master Sep 14, 2022
@danroozemond
Copy link
Contributor Author

Thanks for the quick review and merge, good stuff 🙂

@samuelcolvin
Copy link
Sponsor Contributor

Any volunteers for samuelcolvin/watchfiles#187?

@samuelcolvin
Copy link
Sponsor Contributor

Anyone able to review samuelcolvin/watchfiles#194 which implements the fix discussed in samuelcolvin/watchfiles#187 and confirm it has fixed this error?

I don't have a windows machine, so can't test.

@danroozemond
Copy link
Contributor Author

@samuelcolvin thanks for making the fix! I'm happy to have a look but a bit busy the coming days. Is somewhere over the weekend OK?

@samuelcolvin
Copy link
Sponsor Contributor

Of course, no hurry from my pov.

Thanks.

Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
* add note in Settings on using WatchFiles on WSL

* Update docs/settings.md

Adopt suggested text change from PR#1647

Co-authored-by: Samuel Colvin <samcolvin@gmail.com>

* add a link to watchfiles doc

* Update docs/settings.md

* Use tip admonition for watchfiles env variable

Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Kludex added a commit that referenced this pull request Oct 29, 2022
* add note in Settings on using WatchFiles on WSL

* Update docs/settings.md

Adopt suggested text change from PR#1647

Co-authored-by: Samuel Colvin <samcolvin@gmail.com>

* add a link to watchfiles doc

* Update docs/settings.md

* Use tip admonition for watchfiles env variable

Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
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.

None yet

3 participants