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
propagate configuration from paste.ini #156
Conversation
bf450bc
to
117a61e
Compare
def upd_conf_with_list_item(conf, attr, sdict, sep=' ', parse=str_strip): | ||
values = sdict.pop(attr, None) | ||
if values: | ||
conf[attr] = list(filter(None, map(parse, values.split(sep)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: use list comprehension here, as it is easier to read:
conf[attr] = [parse(v) for v in values.split(sep) if v]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is easier to read but your list comprehension is not exactly equivalent to the code it replaces.
To make it equivalent, we must call parse() twice for each value:
conf[attr] = [parse(v) for v in values.split(sep) if parse(v)]
That's why I opted for the list/map/filter combination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Please let me know if you prefer the list comprehension with the two parse() calls)
Thanks for the contribution! I made a few comments throughout the code, and I'd also like to see a test for this before merging. If you're comfortable with |
Sorry, I'm not familiar with py.test yet (or other testing frameworks for Python). That is something I want to learn, but haven't find the time, yet. I'm leaning towards unittest for my own projects (because it is part of the standard library more than anything else). |
@luismsgomes, would you mind rebasing onto master and pushing this branch so we can verify whether the test failures are due to new things or issues that have already been resolved? |
Hi, I will try my best. I'm not yet familiar with git rebase (my primary versioning system is mercurial). |
Dear @luismsgomes qnother option is to gradually cherry-pick your commits on top of master,
Tip: in general, when creating PRs, it is better to create a separate branch (i.e. |
Dear @ankostis, I might have messed things because I don't quite know how git rebase or cherry-pick works... I did a pull from your master repository and git said it was replaying my changes on top of it. It didn't ask me to merge anything. I did a push and now github says my fork is 22 commits ahead and 22 commits behind, which I don't understand at all. |
No prob, I have fixed it for you. Assuming you are on your
And now your master should point in this commit:
Then just |
requirements/dev.pip
Outdated
@@ -15,7 +15,7 @@ pytest>=2.3 | |||
webtest; python_version != '2.5' | |||
mock; python_version <= '3.2' | |||
gevent>=1.1b4; python_version >= '3' | |||
twine>=1.6.1 | |||
twine>=1.7,<=1.7.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this pinning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this shouldn't be registering as a change, and I don't think it would be with a proper rebase. This is from PR #158, which has already been merged. It was originally added because twine
1.8 breaks our test_server
tests due to a new call signature.
Dear @mplanchard I've just finished my 6-month effort to "release" my daily work, so I haven't fully recovered my slices of free time for this project:-) |
Sure thing @ankostis! I have been busy at work as well (we're migrating an old, huge PHP website to Python and a new infrastructure), but I will find time tonight to do a proper rebase and get this in. |
Done.
Thank you a lot @ankostis!
…On Wed, Feb 15, 2017 at 4:36 PM, Kostis Anagnostopoulos < ***@***.***> wrote:
No prob, I have fixed it for you.
Assuming you are on your master and have no un-committed changes in your
working dir,
just execute the following commands:
git remote add ankostis ***@***.***/ankostis/pypiserver.git
git fetch ankostis
git reset --hard ankostis/proppaste
git log -1
And now your master should point in this commit:
git log -1
commit 86cbda1
Author: Luís Gomes ***@***.***>
Date: Thu Jul 14 09:51:32 2016 +0100
minor refactoring; removed extraneous print()
Then just git push <your-remote> master -f.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#156 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AItRGZ0OvkQlhuOjxGK54pJAyJOuUQZEks5rcyl2gaJpZM4JBs-S>
.
|
@luismsgomes, could you verify that my rebased PR #169 contains the all the changes you would expect? I pulled in master's history by running
Otherwise, it is an exact copy of your changes |
The ability to propagate configuration values from a paste config file was introduced in pypiserver#156. However, as pointed out in pypiserver#125 by @redbaron4, the string strip method introduced in pypiserver#156 was problematic in Python 2. This resolves that issue while also creating a test that fails on the current master and passes with updates, demonstrating the issue.
Here's an example paste.ini:
The following configuration keywords were not being propagated into the app:
Now it works.
Hope you like the code.
I tried to keep same style.