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

new --reload-extra-file option #1527

Merged
merged 1 commit into from Jun 14, 2017

Conversation

Projects
None yet
2 participants
@garbas
Contributor

garbas commented Jun 13, 2017

I quickly added an option which is implemented but not exposed in the command line.
Would something like this be excepted? If yes I'll add the documentation for the command line option.

/cc @tilgovi (author of gunicorn/reloader.py)

@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Jun 14, 2017

Seems okay to me!

@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Jun 14, 2017

As far as I can tell, nothing will break if you don't validate that the files exist, but maybe it's the right idea so that users don't silently watch the wrong thing?

@garbas

This comment has been minimized.

Contributor

garbas commented Jun 14, 2017

@tilgovi I usually follow the "fail early" rule and if somebody provides an option that points to a file which we should watch and that file does not exists it is very likely to be a typo. I would assume 99% of cases is like this. In the other 1%, you can always use reloader.py module programmaticaly and skip the validation.

I added the description for new option and squashed the work in one commit. As far as I can tell this looks ready to be merged, if not let me know what needs to change.

@tilgovi

Looks good to me. Thanks!

I left a few tiny comments for your consideration, but nothing critical.

@@ -379,6 +388,17 @@ def validate_list_string(val):
return [validate_string(v) for v in val]
def validate_list_of_existing_files(val):
if not val:

This comment has been minimized.

@tilgovi

tilgovi Jun 14, 2017

Collaborator

Maybe remove these few lines and replace with a call to validate_list_string?

@@ -368,6 +368,15 @@ def validate_string(val):
return val.strip()
def validate_file_exists(val):
val = validate_string(val)
if val is None:

This comment has been minimized.

@tilgovi

tilgovi Jun 14, 2017

Collaborator

I'm not sure we want this here. For the string options, I think it's there for when the default is None if the option wasn't provided at all. In the case of this option, the default is an empty list.

default = []
desc = """\
Extends --reload option to also watch and reload on additional files
(eg. templates, configurations, specifications, ...).

This comment has been minimized.

@tilgovi

tilgovi Jun 14, 2017

Collaborator

nit: "(e.g., templates, configurations, specifications, etc.)."

@garbas

This comment has been minimized.

Contributor

garbas commented Jun 14, 2017

@tilgovi Thank you for the review. I fixed problems you mentioned in your comments. Does it look ok now?

@tilgovi tilgovi merged commit 5426b04 into benoitc:master Jun 14, 2017

0 of 8 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
couverture-io/py26 Verifying code coverage...
Details
couverture-io/py27 Verifying code coverage...
Details
couverture-io/py34 Verifying code coverage...
Details
couverture-io/py35 Verifying code coverage...
Details
couverture-io/py36 Verifying code coverage...
Details
couverture-io/py36-dev Verifying code coverage...
Details
couverture-io/py37 Verifying code coverage...
Details
@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Jun 14, 2017

Thanks, @garbas. Merged. 👍

@garbas garbas deleted the garbas:reload-extra-files branch Jun 14, 2017

fofanov pushed a commit to fofanov/gunicorn that referenced this pull request Mar 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment