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

Obtain help pager information via SLI and handle failures better #948

Merged
merged 4 commits into from Jun 4, 2018

Conversation

@heplesser
Copy link
Contributor

@heplesser heplesser commented May 2, 2018

This is a replacement for #931, see also discussion there. We now get pager information directly from the SLI interpreter if available. Error handling is also more robust; in the past, if the pager did not work, help would simply do nothing.

@physicalist Could you also review this one?

heplesser added 2 commits May 2, 2018
Systematically search for pager if none defined, with cat as last resort.
Provide error message if pager fails.
Copy link
Collaborator

@steffengraber steffengraber left a comment

Looks good and works for me.

subprocess.call([pager, objf])
try:
subprocess.check_call([pager, objf])
except (FileNotFoundError, subprocess.CalledProcessError):

This comment has been minimized.

@physicalist

physicalist May 3, 2018

Issues I can see here:

  • FileNotFoundError is specific to Python 3.x, so Python 2.7.x will throw a NameError at this point. The common suggestion is to catch IOError instead, but that is a more general exception.
  • since pager might still be None, I'd suggest to wrap the try block into an if pager: block, otherwise the call yields uncaught exceptions with really obscure tracebacks due to the argument being a NoneType:
    Python 3: TypeError: expected str, bytes or os.PathLike object, not NoneType
    Python 2: AttributeError: 'NoneType' object has no attribute 'rfind'

This comment has been minimized.

@heplesser

heplesser May 4, 2018
Author Contributor

@physicalist I now added IOError and OSError; I got the latter when testing under 2.7 with a non-existing pager.
I also restructured the code in show_help_with_pager() to make control flow simpler: Each possible output option (notebook, print(), pager) is now checked for in turn and the function returns as soon as the first case has been handled.

# https://stackoverflow.com/questions/377017
for candidate in ['less', 'more', 'cat']:
if any(os.access(os.path.join(path, candidate), os.X_OK)
for path in os.environ['PATH'].split(os.pathsep)):

This comment has been minimized.

@physicalist

physicalist May 3, 2018

As per the comment by user MestreLion over at StackOverflow, this if clause would evaluate to true even if the candidate is a directory, so ideally one should probably add the os.path.isfile check even if it's an extremely unlikely case to have a directory of the same name as a popular UNIX utility appear in the PATH before the appropriate binary file. Unfortunately it makes the code more convoluted and harder to read as well, maybe use a helper function here?

def __is_executable(path, candidate):
    return os.access(os.path.join(path, candidate), os.X_OK) and os.path.isfile(os.path.join(path, candidate))

This comment has been minimized.

@heplesser

heplesser May 4, 2018
Author Contributor

Thanks, implemented!

@heplesser
Copy link
Contributor Author

@heplesser heplesser commented May 4, 2018

@physicalist Thank you for your suggestions, I had overlooked those cases. They are integrated now, together with a bit of restructuring.

@heplesser
Copy link
Contributor Author

@heplesser heplesser commented May 14, 2018

@physicalist Could you check if you are happy with my changes and approve the PR if you are?

@heplesser heplesser requested review from gtrensch and removed request for Silmathoron May 28, 2018
Copy link
Collaborator

@gtrensch gtrensch left a comment

Looks good to me!

@heplesser heplesser merged commit 4a99001 into nest:master Jun 4, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@heplesser heplesser deleted the heplesser:fix-help-pager-nestrc branch Aug 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.