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

Update HOHQMeshMain.f90 #121

Merged
merged 6 commits into from
Dec 6, 2024
Merged

Update HOHQMeshMain.f90 #121

merged 6 commits into from
Dec 6, 2024

Conversation

DavidAKopriva
Copy link
Collaborator

Add help to be printed with the -help option.

Add help to be printed with the -help option.
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 96.15385% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.83%. Comparing base (51511c2) to head (31be833).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
Source/HOHQMeshMain.f90 95.55% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #121      +/-   ##
==========================================
+ Coverage   75.05%   75.83%   +0.77%     
==========================================
  Files          68       68              
  Lines       10519    10568      +49     
  Branches        2        2              
==========================================
+ Hits         7895     8014     +119     
+ Misses       2624     2554      -70     
Flag Coverage Δ
unittests 75.83% <96.15%> (+0.77%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Source/HOHQMeshMain.f90 Show resolved Hide resolved
WRITE(OUTPUT_UNIT,*) "Invocation:"
WRITE(OUTPUT_UNIT,*) " ./HOHQMesh [options]"
WRITE(OUTPUT_UNIT,*) "Options:"
WRITE(OUTPUT_UNIT,*) "-help"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
WRITE(OUTPUT_UNIT,*) "-help"
WRITE(OUTPUT_UNIT,*) "-help or --help"

For consistency if both commands are allowed.

Copy link

@fhindenlang fhindenlang Dec 5, 2024

Choose a reason for hiding this comment

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

best is to also check if all command line arguments could be associated, and if not, print the help instead.
so if someone would now type -h, which is not part of the possible arguments, then the help would still appear.
From the argument syntax, I am more used to a minus with one letter, like -f followed by space and value, or an equivalent long argument like --file followed by = and value.
All this functionality would be provided by CLAF90, you should give it a try...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A couple of issues with CLAF90, though. 1. No license. 2. We then depend on them maintaining it. I have been burned too many times over the years by libraries that stop being maintained and hence avoid them.

Choose a reason for hiding this comment

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

well the license says that you can copy it, so I just copied it into my code, its not something that need maintenance. But no worries, its your choice ;)

Now accepts -h, -help, --help for help. Prints out help when there is an error in the command line arguments.
@DavidAKopriva
Copy link
Collaborator Author

OK, I've added three options to get help: -help, --help and -h. The help message gets printed if there is an error.

Source/HOHQMeshMain.f90 Outdated Show resolved Hide resolved
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

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

With the one redundancy removed, this LGTM!

@DavidAKopriva
Copy link
Collaborator Author

I still need to put in some test coverage. Will do that next.

Remove -h option. Add comment that partial command line aregumens are handled.
Add a MiscTests procedure to put in miscellaneous tests from now on. Right now, it calls the PrintHelpMessage procedure to scratch to include covereage.
@DavidAKopriva
Copy link
Collaborator Author

It passes codecov/project but not codecov/patch. Not sure what that means, and I'm not sure how to test command line arguments when those are passed through from the user. Unless there is some way to spoof the CLAs.

@sloede
Copy link
Member

sloede commented Dec 6, 2024

I improved coverage in cddf7f7. LGTM now!

Copy link
Member

@andrewwinters5000 andrewwinters5000 left a comment

Choose a reason for hiding this comment

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

LGTM!

@sloede sloede merged commit f49ddf8 into main Dec 6, 2024
21 checks passed
@sloede sloede deleted the JossRev2 branch December 6, 2024 07:29
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.

4 participants