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

Clean up help string #36

Merged
merged 6 commits into from
May 7, 2021
Merged

Clean up help string #36

merged 6 commits into from
May 7, 2021

Conversation

ewu63
Copy link
Collaborator

@ewu63 ewu63 commented May 4, 2021

Purpose

This is an attempt at cleaning up the help string:

  • correctly use the raw formatter. No newlines are needed if triple quotes are used
  • use triple quotes to auto line wrap without manual insertion of \n
  • set the display width to 120 characters via overloading of environment variables. Lines longer than this are wrapped (unless raw formatting is used)
  • replaced hard-coded default values in help string with placeholder

There are plenty of things that we can improve:

  • Rename some "modes" to use camelCase for readability (this will break backwards compatibility probably unless we allow any case and uniformly cast to lower)
  • print more defaults for other options. We will have to come up with a smart way to do this without printing a lot of (default: None)
  • Fix some defaults. For some stuff (e.g. outFile for timecombine the default is None, but later on we just replace it with another default. We should probably just set the default in the argparser directly)
  • Fix display of help string. Right now, the help string for each mode is shown with cgns_utils -h but not when specific modes are queried. Ideally, they should be shown in both places and the description= option may do this.

There is also an open question of whether we want a docs page for this. Some of the help text spans too many lines, so maybe they should go into a docs page instead? For example, I don't think it's necessary to describe the structure of some input files via the help string.

Type of change

What types of change is it?
Select the appropriate type(s) that describe this PR

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Existing tests pass. There are no functionality changes and I did manually check a lot of the help text.

Checklist

Put an x in the boxes that apply.

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@ewu63 ewu63 requested a review from a team as a code owner May 4, 2021 21:28
marcomangano
marcomangano previously approved these changes May 5, 2021
Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

I agree with all your points, I just wonder how we want to organize this effort - namely, who is going to put in all the work.
I would prioritize the help string fixes and (if we decide to go for it) work on the documentation at a later stage. For the relatively little experience I have with cgnsutilities I think that the added value for an RTD page would mostly be the possibility to add some explanatory graphics... right?

@ewu63
Copy link
Collaborator Author

ewu63 commented May 5, 2021

I agree with all your points, I just wonder how we want to organize this effort - namely, who is going to put in all the work.
I would prioritize the help string fixes and (if we decide to go for it) work on the documentation at a later stage. For the relatively little experience I have with cgnsutilities I think that the added value for an RTD page would mostly be the possibility to add some explanatory graphics... right?

Yeah maybe others have different thoughts on priority, but to me 2 and 4 are the most important, but will take some time. As for docs, I was thinking some of the help string should be moved into the docs page instead (especially the really long strings that explain input file formats).

On the other hand, maybe we want to focus on adding tests instead? So far very little code is tested.

Copy link
Contributor

@eirikurj eirikurj left a comment

Choose a reason for hiding this comment

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

Fixed couple of non-issues, Approving and merging.
I agree we can improve more, and maybe we should just create issues for those to discuss and prioritize?
To answer briefly here some points:

  • Yeah we should probably unify the options. Many already have camelCase.
  • We should at least add the defaults that are already specified to the help string of a given option.
  • Improving defaults would be helpful.
  • Right, currently this is set up in this hierarchical manner so its assumed that you read the top level description. This is not the best design. We should include the short description from the top level also in the actual option help.

Since this is a CLI tool, in the past we baked a minimum documentation and examples into some help strings for some suboptions.
If we want to improve documentation I think we can make a proper sphinx docs page, or is it sufficient to instead make extensive example scripts (since the examples document the usage)? Ideally we should have both.
I think as we include/add examples we can alongside add tests.

@eirikurj eirikurj merged commit 2a0a7aa into master May 7, 2021
@eirikurj eirikurj deleted the update-cli-help branch May 7, 2021 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants