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.

Sign up for GitHub

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

Jump to bottom

Prefix config file with its type #1068

Merged
merged 1 commit into from
Jul 9, 2015
Merged

Prefix config file with its type #1068

merged 1 commit into from
Jul 9, 2015

Conversation

slava-sh
Copy link
Contributor

@slava-sh slava-sh commented Jul 7, 2015

Fixes #836 as described in @benoitc's comment.

berkerpeksag
berkerpeksag reviewed Jul 7, 2015
View reviewed changes
tests/test_003-config.py
def test_load_config_module():
with AltArgs(["prog_name", "-c", cfg_module()]):
with AltArgs(["prog_name", "-c", "python:%s" % cfg_module()]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be nice to keep the test untouched to make sure that we don't break backwards compatibility. We can add a separate test for the python: prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding of @benoitc's comment, we do want to break backward compatibility here in order to separate the two cases. What do you think?

@berkerpeksag
Copy link
Collaborator

Thanks for the PR, @slava-sh :)

Three comments:

  • Maybe we can also add py: as an alternative to python:.
  • Can you please squash the commits into one when you're done?
  • An additional "thank you" for adding tests :)

@slava-sh
Copy link
Contributor Author

slava-sh commented Jul 7, 2015

I don't see a point in having two different prefixes for doing one thing. We can choose a prefix different from python: though.

@tilgovi
Copy link
Collaborator

tilgovi commented Jul 7, 2015

👍

No opinion on python vs py. Both are fine. We should pick one.

@benoitc
Copy link
Owner

benoitc commented Jul 7, 2015

@berkerpeksag why py instead of python? Shorter?

@slava-sh
Copy link
Contributor Author

slava-sh commented Jul 7, 2015

How about module?

@tilgovi
Copy link
Collaborator

tilgovi commented Jul 7, 2015

An alternative idea: use a colon to distinguish between paths and modules, with the colon separating the package from the relative path.

Colon is uncommon in filenames because it's not very portable, so it's likely unambiguous.

Pyramid does this and calls them asset specifications.

"foo/bar" is a relative path
"/path/to/foo/bar" is an absolute path
"foo:bar" is the "bar" path within the "foo" package.

@benoitc
Copy link
Owner

benoitc commented Jul 7, 2015

@tilgovi interesting idea. IF the pattern is already used why not. I was just reusing the pattern existing in trac and other stuff like this.

@berkerpeksag
Copy link
Collaborator

@berkerpeksag why py instead of python? Shorter?

Yes.

I also like @tilgovi's suggestion since it's already being used in the wild.

@tilgovi
Copy link
Collaborator

tilgovi commented Jul 7, 2015

I don't have a preference. Just mentioning it. I haven't seen it anywhere other than Pyramid. If Trac and others have file: that's fine, too. Paste uses config: and egg:. Anything is acceptable.

@tilgovi
Copy link
Collaborator

tilgovi commented Jul 7, 2015

I think I like file: and python: (or module:) since there is no guessing about the meaning. It is very explicit.

@benoitc
Copy link
Owner

benoitc commented Jul 7, 2015

i would go for python: the code already exists :)
On Tue 7 Jul 2015 at 21:51 Randall Leeds notifications@github.com wrote:

I think I like file: and python: (or module:) since there is no guessing
about the meaning. It is very explicit.

—
Reply to this email directly or view it on GitHub
#1068 (comment).

@tilgovi
Copy link
Collaborator

tilgovi commented Jul 8, 2015

👍

@tilgovi
Copy link
Collaborator

tilgovi commented Jul 8, 2015

Needs a rebase, though.

@slava-sh
Copy link
Contributor Author

slava-sh commented Jul 9, 2015

I think this can be merged now.

@benoitc
Copy link
Owner

benoitc commented Jul 9, 2015

LGTM

berkerpeksag added a commit that referenced this pull request Jul 9, 2015
@berkerpeksag
Merge pull request #1068 from slava-sh/836-add-config-type-prefix
ec3664d 
Prefix config file with its type

Fixes #836
@berkerpeksag berkerpeksag merged commit ec3664d into benoitc:master Jul 9, 2015
@berkerpeksag
Copy link
Collaborator

Thanks!

yyuu added a commit to yyuu/gunicorn that referenced this pull request Apr 11, 2017
@yyuu
Fallback to the original behavior if prefix is not available (benoitc…
e0171e8 
…#1068)

I needed to restore the original behavior because some external
applications was actually relying on the behavior.

https://github.com/apache/incubator-airflow/blob/1.8.0/airflow/bin/cli.py#L775
yyuu added a commit to yyuu/incubator-airflow that referenced this pull request Apr 12, 2017
@yyuu
[AIRFLOW-1102] Add 'python:' to gunicorn config
1e09f13 
This is necessary to make `airflow webserver` working
with `gunicorn` 19.4+ (benoitc/gunicorn#1068)
@benoitc benoitc mentioned this pull request Oct 26, 2017
Closed
mjjbell pushed a commit to mjjbell/gunicorn that referenced this pull request Mar 16, 2018
@berkerpeksag
Merge pull request benoitc#1068 from slava-sh/836-add-config-type-prefix
736e4b1 
Prefix config file with its type

Fixes benoitc#836
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@slava-sh @berkerpeksag @tilgovi @benoitc