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

Feature 1527 log second try #1558

Closed
wants to merge 1 commit into from
Closed

Conversation

JohnHalleyGotway
Copy link
Collaborator

Pull Request Testing

I found a problem with the code changes for #1527. In command_line.cc, do_verbosity() and do_log() only parsed the LAST instance of the -v and -log options, respectively. If they are used more than once on the command line, the extra ones remain in the list but the calling application doesn't know how to parse them. Fix this by having the do_verbosity() and do_log() parse and remove all of them from the command line object in the order provided.

  • Describe testing already performed for these changes:

    Tested by running:
met/bin/plot_point_obs met/out/pb2nc/sample_pb.nc plot.ps -v 1 -v 2 -log a -log b

Prior to the fix, this errors out. After the fix, it runs fine. This matches the behavior in the main_v9.1 branch.

  • Recommend testing for the reviewer to perform, including the location of input datasets:

    Review code changes. Run a couple of tests to confirm that the fix makes -v and -log work the same as they do in MET version 9.1.

  • Will this PR result in changes to the test suite? [No]

    If yes, describe the new output and/or changes to the existing output:

  • After merging, should the reviewer DELETE the feature branch from GitHub? [Yes]

Pull Request Checklist

See the METplus Workflow for details.

  • Complete the PR definition above.
  • Ensure the PR title matches the feature or bugfix branch name.
  • Define the PR metadata, as permissions allow.
    Select: Reviewer(s), Project(s), and Milestone
  • After submitting the PR, select Linked Issues with the original issue number.

…mmand_line.cc parses and removes only the last instance of -v and -log from the command line. If they were used more than once, then the extra ones are left in the command line object and the calling application doesn't know how to parse them. Fix this by having the do_verbosity() and do_log() parse and remove all of them from the command line object in the order provided.
@JohnHalleyGotway JohnHalleyGotway added this to the MET 10.0 milestone Nov 13, 2020
@JohnHalleyGotway JohnHalleyGotway added this to In progress in MET-10.0.0-beta2 (12/7/20) via automation Nov 13, 2020
@JohnHalleyGotway JohnHalleyGotway linked an issue Nov 13, 2020 that may be closed by this pull request
20 tasks
@jprestop
Copy link
Collaborator

I review code changes.

I ran:
met/bin/plot_point_obs met/out/pb2nc/sample_pb.nc plot.ps -v 1 -v 2 -log a -log b
in the existing /d1/projects/MET/MET_pull_requests/met-10.0_beta2/feature_1527/MET-feature_1527_log_into_develop (pre-changes) directory and received the usage statement for plot_point_obs.

I ran the same command in /d1/projects/MET/MET_pull_requests/met-9.1_beta4/MET-feature_1447_tc_gen_into_develop/ to see what the behavior would be with 9.1.

I ran the same command in the newly created /d1/projects/MET/MET_pull_requests/met-10.0_beta2/feature_1527_log/MET-feature_1527_log_into_develop (post changes) directory and saw that the behavior was the same as for met-9.1 and the command ran successfully.

Changes are clear and concise.

@jprestop jprestop closed this Nov 13, 2020
MET-10.0.0-beta2 (12/7/20) automation moved this from In progress to Done Nov 13, 2020
@jprestop jprestop deleted the feature_1527_log branch November 13, 2020 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Parse -v and -log command line options first.
2 participants