Feature: Change reload to be configurable with glob patterns

[Roang-zero1]

With this change it is possible to control which files will trigger a reload of uvicorn.

Features:

• New Option reload-include is a repeatable list of unix style glob patterns pathlib.Path.glob that will be included in reload watch.
• New Option reload-exclude is a repeatable list of unix style glob patterns pathlib.Path.glob that will be excluded in reload watch.
• Common Watchgod and Startreload configuration:
  • Both reloaders get their directories from the new configurations
• Improved Watchgod implementation:
  • Use should_watch_dir to reduce setup overhead
  • Use should_watch_file with pathlib to verify included paths
  • Only watch .py files by default
• Improved Startreload implementation:
  • Use pathlib glob to search for .py files
• Updated documentation
• Updated tests

Edit by @Kludex:

• Closes Support --ignore or --reload-file option #984
• Closes Ignore changes to any top-level directory starting with a "." #966

[euri10]

thanks for this !

I begun reviewing this and realize it would be simpler to split it in 2:

1. the test refactor
2. the added functionality

in no particular order, this one seems simpler from my point of view.

Sorry but cant wrap my head around reviewing that much to be honest, and in any case it's a separate issue.

I left my comments anyway since I begun but the split would be much appreciated

[Roang-zero1]

Sure, I'll start the work on separating out the test refactor.
I'll keep this PR for the reimplementation of the reload function and start a new one for the test refactor.

[Roang-zero1]

Here are some points of consideration

[Roang-zero1]

As requested I've separated out the test class refactor to #833.

Ready for new round of review. Added some comments to point out where feedback is requesed.

[euri10]

Will look at it over the weekend and get back to you, thanks !

…

On Fri, Oct 23, 2020, 7:22 PM Lucas ***@***.***> wrote: As requested I've separated out the test class refactor to #833 <#833>. Ready for new round of review. Added some comments to point out where feedback is requesed. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#820 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAINSPX5VCEQNWOXUTB4SMDSMG3W7ANCNFSM4ST3ARHA> .

[hanneskuettner]

Any updates on this PR?

[Roang-zero1]

Just got the notification. I would be willing to revisit this PR, just let me know what is needed.

[Kludex]

Hi @Roang-zero1 👋

I'm going to review it over the weekend. If you'd like, merging master would be great! 😄

Sorry for the delay on it!

[kennylajara]

I'm going to review it over the weekend.

Bump @Kludex

[Roang-zero1]

I've added the changes, unfortunately this meant reworking large parts of this PR 🔨. (Sorry @euri10 for the additional work)
There is now only one watcher for the current working directory. This enables watchgod to pick up new files and directories that match the globs that were not present before starting the server.

I think I've added test cases for all new functionality.
I am a bit on the fence as there is some feature duplication between config.py and the watchgodreload.py, but I believe this to be unavoidable.

I've added a cache to the watchers, this reduces the CPU load, but costs memory. Please indicate if that is ok.

There is now a fixture for testing all the reload related use cases, it is currently scoped for package, as I don't have a good enough understanding please verify it the scope is correct.

[Roang-zero1]

Just saw the failed test. I'll look into what the problem with Windows is tomorrow.

[euri10]

this is a subtle one:
this test works fine with this logger: logger.info(f"Will watch for changes in these directories: {list(map(str, self.reload_dirs))}")

        assert (
            caplog.records[-1].message
            == f"Will watch for changes in these directories: {[str(app_dir)]}"
        )

you cannot build manually the list [] I think that was what caused the double escaping

[euri10]

for the failing test, let's deal with that sorting the files in the output for predictable behaviour:

diff --git a/tests/test_config.py b/tests/test_config.py
index a999b33..73f2e5f 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -225,8 +225,10 @@ def test_reload_includes_exclude_dir_patterns_are_matched(
                 "Will watch for changes in these directories: "
                 in caplog.records[-1].message
             )
-            assert str(first_app_dir) in caplog.records[-1].message
-            assert str(second_app_dir) in caplog.records[-1].message
+            assert (
+                    caplog.records[-1].message
+                    == f"Will watch for changes in these directories: {sorted([str(first_app_dir), str(second_app_dir)])}"
+            )
             assert frozenset(config.reload_dirs) == frozenset(
                 [first_app_dir, second_app_dir]
             )

             )
             )
diff --git a/uvicorn/config.py b/uvicorn/config.py
index 4fae430..13f5898 100644
--- a/uvicorn/config.py
+++ b/uvicorn/config.py
@@ -331,7 +331,7 @@ class Config:

             logger.info(
                 "Will watch for changes in these directories: %s",
-                list(map(str, self.reload_dirs)),
+                sorted(list(map(str, self.reload_dirs))),
             )

         if env_file is not None:

[euri10]

thanks a lot @Roang-zero1 for this, this is a very nice enhancement !

[HansBrende]

@Roang-zero1 @euri10 @Kludex this PR introduced a regression and deleted the test for it (test_reload_dir_is_set()). Previously you could pass reload_dirs a string instead of a list. Now:

Config('myapp:app', reload=True, reload_dirs='src')

prints:

WARNING:  Provided reload directories ['r', 'c', 's'] did not contain valid directories, watching current working directory.

https://github.com/encode/uvicorn/pull/820/files#diff-6982a5e270879281f9182303a9da9776148857b46325afb230e50ef8eb3264c9R294
(Of course, set expects an iterable... and strings are iterables of chars.)