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.

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

[flare] add stderr of commands #1586

Merged
merged 2 commits into from
May 4, 2015
Merged

[flare] add stderr of commands #1586

merged 2 commits into from
May 4, 2015

Conversation

degemer
Copy link
Member

@degemer degemer commented Apr 30, 2015

Also:

  • check if files are readable before trying to add them to the
    tarfile.
  • remove whitespaces from input before checking for yes/no.

@degemer degemer added the core label Apr 30, 2015
@degemer degemer added this to the 5.4.0 milestone Apr 30, 2015
choice = raw_input('Are you sure you want to continue [y/N]? ').lower()
if choice not in ['yes', 'y']:
choice = raw_input('Are you sure you want to continue [y/N]? ')
if "".join(choice.lower().split()) not in ['yes', 'y']:
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of choice.strip().lower() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I chose this version for this 🔨 :

In [1]: ' y e s'.strip().lower()
Out[1]: 'y e s'

In [2]: ''.join(' y e s'.lower().split())
Out[2]: 'yes'

But I agree, it doesn't really make sense, I will change both of them.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say if someone typed y e s he probably had one too many 🍻 and need to hold off sending his logs 😉

degemer and others added 2 commits May 4, 2015 11:31
Also:
- check if files are readable before trying to add them to the
tarfile.
- remove whitespaces from input before checking for yes/no.
When launching external commands that import other python modules they
might affect our logging handlers and interfere with them.
This resets them after the call and ensure we keep the same.
@degemer
Copy link
Member Author

degemer commented May 4, 2015

Thanks for the review and the fix @LeoCavaille ! (tested and it works great)
Updated the choice.strip().lower(), will merge if Travis is 🆗 .

@degemer
Copy link
Member Author

degemer commented May 4, 2015

Travis ✅

degemer added a commit that referenced this pull request May 4, 2015
@degemer degemer merged commit cc3c725 into master May 4, 2015
@degemer degemer deleted the quentin/improve-flare branch May 4, 2015 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants