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

Return help text #913

Merged
merged 6 commits into from
Apr 16, 2018
Merged

Return help text #913

merged 6 commits into from
Apr 16, 2018

Conversation

babsey
Copy link
Contributor

@babsey babsey commented Mar 19, 2018

The purpose of the PR is that the function 'help' is able to return text about an object instead of opening in a pager.

Example:
txt = nest.help('iaf_psc_alpha', return_text=True)
print txt

@heplesser heplesser added ZC: Documentation DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Mar 19, 2018
@heplesser heplesser self-requested a review March 19, 2018 11:15
Correct intentation
@babsey babsey force-pushed the Return_help_text branch from 4054f6e to f475b98 Compare March 19, 2018 12:30
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

Looks generally fine, just two small comments in the code.

"""
hlpobj = obj
if hlpobj is not None:
show_help_with_pager(hlpobj, pager)
if isinstance(return_text, str):
Copy link
Contributor

Choose a reason for hiding this comment

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

@babsey Why would you allow return_text to be anything else than True or False?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your question has a good point. Returning help text was my intention for accessing to nest via web protocol. Here, it requests nest.help with return_text='true' and the type of return_text is string. Thus, it should be changed to boolean type.

By the way, an example for the usage of nest.help via web request can be found here:
http://192.52.2.171/ (this link is temporally available)
This page calls several nest methods e.g. nest.Models, nest.help and nest.GetDefaults and renders to webpage dynamically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks nice! I don't know how you web app precisely interfaces with NEST, but I suppose it calls nest.help() from Python, and the web app will always call it with return_value==True. Why can't you then place the Python constant True in the call? I am a bit afraid that details of an external app get too deeply into NEST's code. �@jougs @apeyser Any comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the lines for converting return_text to boolean. Even it works even when return_text contains any non-empty string. So, converting is unnecessary. I do not want to change default output of help function. It should remain opening a pager.

Parameters
----------
hlpobj : object
Object to display
Copy link
Contributor

Choose a reason for hiding this comment

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

"Object to display help for"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected it.

@heplesser heplesser requested a review from steffengraber March 20, 2018 09:00
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

LGTM and merging. Many thanks for the contribution.

@jougs jougs merged commit a3335b9 into nest:master Apr 16, 2018
@babsey babsey deleted the Return_help_text branch April 16, 2018 07:46
@babsey
Copy link
Contributor Author

babsey commented Apr 16, 2018

Thanks for approving. However, few days ago I realized that the function load_help return either one or two variables, which is not a good programming.

In case that the file not found, the function doesnt return any variable. It might call an error, when for this output would split but it is not iterable (a,b = None).

I have already fixed that problem, but I will write solution codes in a new branch and pull the request to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. ZC: Documentation DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants