feat: better reload options

[iamkhav]

Hey all,
I based this PR off of @Andrew-Chen-Wang's work and included his original commit from #225.

Additionally to the rebase, I made a few changes to the descriptions (as required in the previous PR) and added the capability to pass a reload_filter from the Python API.

The reload_filter should be an obj based on watchfiles' BaseClass.

Closes #353 and #225.

[gi0baro]

Left some comments; vast majority of them are nits.

[iamkhav]

Thank you very much for your review :)

I'll have to make another manual testing round later on, haven't spent a lot of time on that after the last two commits.

@gi0baro, should we speak about unit tests for some functionalities? I could cover reload options in an early iteration.
I sent you an email with contact info a few days ago.

[gi0baro]

I'll have to make another manual testing round later on, haven't spent a lot of time on that after the last two commits.

Gonna make another code review ASAP.

@gi0baro, should we speak about unit tests for some functionalities? I could cover reload options in an early iteration.

If you figure out a smart way to unit-test reload options, then yes please, go ahead. In general it's hard to unit-test Granian for everything which regards actual HTTP requests/application interaction (given also how the codebase is structured right now), maybe with some huge refactoring we can change this, but at the moment it feels too costly to be worth (especially considering there are 0 contributors on the Rust side at the moment except for myself). But again, given the context, I think it is a good candidate for unit.

I sent you an email with contact info a few days ago.

Yep, I see it. I just need to find some time to proper reply :)

[gi0baro]

Left some suggestions/comments.
Also, probably changes to README.md need an update too.

[iamkhav]

I'm sorry for the delay on this PR @gi0baro!

Meanwhile, I was thinking a bit about tests.

1. Launch Granian instance with certain reload params
2. Confirm default behaviour for a route (optional)
3. Replace a source file to make that route respond with a specific body
4. Send a request to Granian instance and check if app successfully reloaded by asserting the specific modified body

_{E.g. 2. could be an assert for the response body not to be the later modified one.}

I think we could pull that off with minimal changes to tests/apps.
Happy to attempt this either here or in a subsequent PR (but probably before 1.6).

WDYT?

[gi0baro]

I'm sorry for the delay on this PR @gi0baro!

np at all, there's no need to rush here :)

Meanwhile, I was thinking a bit about tests.

1. Launch Granian instance with certain reload params
2. Confirm default behaviour for a route (optional)
3. Replace a source file to make that route respond with a specific body
4. Send a request to Granian instance and check if app successfully reloaded by asserting the specific modified body

E.g. 2. could be an assert for the response body not to be the later modified one.

This sounds more like testing watchfiles itself rather than Granian to me.
I would probably approach this with a different strategy, unit-testing specifically:

• the final merged params are indeed what we expect in Granian instance
• the change trigger works
  I think we can implement those tests without the need of making a request/response flow, as refactoring Granian class to mock the startup and the reload trigger should be relatively easy.

Happy to attempt this either here or in a subsequent PR (but probably before 1.6).

I'm definitely more up to a separate PR for tests, planning to review and merge this one in the near future.
I don't have any specific ETA for 1.6, so feel free to take your time; we can discuss the single PRs individually for 1.6 inclusion :)

[iamkhav]

Quick note because I stumbled over this while testing the feature.

I had also assumed that this will regex match on the full path of something that triggered the reload. However, this only matches the last part of the path, meaning the exact file/dir that triggered the reload, not the entire path.

https://watchfiles.helpmanual.io/api/filters/#watchfiles.DefaultFilter.ignore_entity_patterns

"Entity" isn't a much better description 😛 but "Path" might be suggesting a different behaviour altogether. WDYT?

_{I want to avoid issues where users report this to be broken.}

[gi0baro]

Oh, I missed that. Would Relative path patterns make sense to you?

[iamkhav]

Sorry for the late answer.

I think being fully explicit could help.

Suggested change

	'Path patterns (regex) to ignore changes for. '
	'File/directory name patterns (regex) to ignore changes for. '

WDYT?

[gi0baro]

Looks perfect. Can you open a PR for this? otherwise I'll do it by myself later

[iamkhav]

No worries, will do!