Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

Update pulp-manage-db to handle default answer, closes #2467 #2874

Closed
wants to merge 1 commit into from
Closed

Update pulp-manage-db to handle default answer, closes #2467 #2874

wants to merge 1 commit into from

Conversation

elyezer
Copy link
Contributor

@elyezer elyezer commented Dec 5, 2016

Default to no when any input is given for the continue question when running
pulp-manage-db.

I did some testing and the new condition works:

>>> not len(a) == 0 and a[0] == 'y'
False
>>> a='n'; not len(a) == 0 and a[0] == 'y'
False
>>> a=''; not len(a) == 0 and a[0] == 'y'
False
>>> a='y'; not len(a) == 0 and a[0] == 'y'
True
>>> a='    '; not len(a) == 0 and a[0] == 'y'
False

Fixes https://pulp.plan.io/issues/2467

@mention-bot
Copy link

@elyezer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @werwty, @jeremycline and @mhrivnak to be potential reviewers.

@elyezer
Copy link
Contributor Author

elyezer commented Dec 5, 2016

I wanted to write tests for the function but I was not sure where to add them. Let me know and I will be happy to add some unit tests in order to ensure all it is going to work as expected.

@bmbouter
Copy link
Member

bmbouter commented Dec 5, 2016

@elyezer If this PR came with no tests that would be fine with me. Also can you paste a link to this PR on the issue and transition the issue to POST which is the correct state for issues which have a PR associated with them.

@bmbouter
Copy link
Member

bmbouter commented Dec 5, 2016

@elyezer Also can you update the commit to use the closes syntax which will associate the commit with the issue in Redmine when merged. See the new commit messages section for details.

http://docs.pulpproject.org/en/2.12/nightly/dev-guide/contributing/branching.html#commit-messages

@elyezer
Copy link
Contributor Author

elyezer commented Dec 5, 2016

@elyezer Also can you update the commit to use the closes syntax which will associate the commit with the issue in Redmine when merged. See the new commit messages section for details.

Definitely, I was expecting for a comment like this. Thanks for pointing me out to the proper documentation.

@@ -294,4 +294,4 @@ def _start_logging():

def _user_input_continue(question):
reply = str(raw_input(_(question + ' (y/N): '))).lower().strip()
return reply[0] == 'y'
return not len(reply) == 0 and reply[0] == 'y'
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be better as:

try:
    return reply[0] == 'y'
except IndexError:
    return False

Copy link
Contributor Author

@elyezer elyezer Dec 5, 2016

Choose a reason for hiding this comment

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

I thought using conditions would be much faster than try/catch but that is not the case:

In [7]: def test1(s):
   ...:     try:
   ...:         return s[0] == 'y'
   ...:     except IndexError:
   ...:         return False
   ...:     

In [10]: def test2(s):
    ...:     return not len(s) == 0 and s[0] == 'y'
    ...: 

In [14]: %timeit "test1('')"
100000000 loops, best of 3: 7.81 ns per loop

In [15]: %timeit "test2('')"
100000000 loops, best of 3: 7.75 ns per loop

I will update the code.

Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

Thank you for putting in a PR on this! Can you update in the way suggested please? I think it's more Pythonic.

Proper handle the default/highlighted answer (empty input) for the continue
question asked when workers are still running.

Closes #2467
@elyezer
Copy link
Contributor Author

elyezer commented Dec 5, 2016

@bmbouter updated the PR.

Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

Thanks @elyezer ! Also when this is merged consider setting the target platform release to 2.11.1 which I believe is what 2.11-dev currently is.

Also here is a link you may find interesting on the general prinicple of "EAFP".

@elyezer elyezer changed the title Fix 2467 Update pulp-manage-db to handle default answer, closesFix 2467 Dec 6, 2016
@elyezer elyezer changed the title Update pulp-manage-db to handle default answer, closesFix 2467 Update pulp-manage-db to handle default answer, closes #2467 Dec 6, 2016
@werwty
Copy link
Contributor

werwty commented Dec 7, 2016

Since we removed the feature in #2883 I am closing this PR.

@werwty werwty closed this Dec 7, 2016
@elyezer elyezer deleted the fix-2467 branch December 12, 2016 17:08
goosemania added a commit to goosemania/pulp that referenced this pull request Jul 21, 2017
Race condition was possible when applicability generation was running
in parallel for the same consumer profiles.

closes pulp#2874
https://pulp.plan.io/issues/2874
goosemania added a commit to goosemania/pulp that referenced this pull request Jul 21, 2017
Race condition was possible when applicability generation was running
in parallel for the same consumer profiles.

closes pulp#2874
https://pulp.plan.io/issues/2874
goosemania added a commit to goosemania/pulp that referenced this pull request Jul 21, 2017
Race condition was possible when applicability generation was running
in parallel for the same consumer profiles.

closes pulp#2874
https://pulp.plan.io/issues/2874
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants