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

Updating timezone cli call #1551

Merged
merged 7 commits into from
Sep 21, 2021

Conversation

BelligerG
Copy link
Contributor

@BelligerG BelligerG commented Sep 17, 2021

The CLIs for improver generally have the ancil files loaded as the last file. This updates the CLI to match the others within the suite.

Testing:

  • Ran tests and they passed OK
  • Added new tests for the new feature(s)

@codecov
Copy link

codecov bot commented Sep 17, 2021

Codecov Report

Merging #1551 (497ffa5) into master (0aad796) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1551   +/-   ##
=======================================
  Coverage   97.93%   97.93%           
=======================================
  Files         106      106           
  Lines        9463     9463           
=======================================
  Hits         9268     9268           
  Misses        195      195           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0aad796...497ffa5. Read the comment docs.

@BelligerG BelligerG marked this pull request as ready for review September 17, 2021 13:38
@tjtg tjtg added the BoM review required PRs opened by non-BoM developers that require a BoM review label Sep 19, 2021
tjtg
tjtg previously approved these changes Sep 20, 2021
Copy link
Contributor

@tjtg tjtg left a comment

Choose a reason for hiding this comment

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

Changes to re-order arguments for better consistency with other CLIs make sense. We're not currently using this CLI at BOM, so this change won't have any impact on our suites.

@BelligerG
Copy link
Contributor Author

This has become a bit of a hacky fix, to make this more robust in the future we should consider altering the interface as per #1553

Copy link
Contributor

@Kat-90 Kat-90 left a comment

Choose a reason for hiding this comment

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

All looks good to me.

@benfitzpatrick
Copy link
Contributor

No acceptance tests for this CLI in master, although we should consider it.

Copy link
Contributor

@benfitzpatrick benfitzpatrick left a comment

Choose a reason for hiding this comment

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

Looks fine, works for the suite with the corresponding suite PR.

@benfitzpatrick benfitzpatrick merged commit ad48bb7 into metoppv:master Sep 21, 2021
MoseleyS added a commit to MoseleyS/improver that referenced this pull request Sep 23, 2021
* master:
  Adds land-mask option to threshold CLI (metoppv#1559)
  Move statsmodels in environment yml files (metoppv#1556)
  rearranged args (metoppv#1558)
  Update Code-Style-Guide.rst (metoppv#1554)
  Updating timezone cli call (metoppv#1551)
MoseleyS added a commit to MoseleyS/improver that referenced this pull request Oct 5, 2021
* bayliffe/mobt77:
  Review changes. Acceptance test added.
  Add type-hinting throughout.
  Formatting.
  Adding unit tests.
  Grouping solution and rename.
  First draft of a modal symbol plugin.
  Modifies LightningFromCapePrecip plugin to accept 3h precipitation-rate-max data (metoppv#1568)
  Moves position of land-sea-mask in threshold CLI (metoppv#1565)
  Re-label a diagnostic as a period diagnostic (metoppv#1561)
  Adds LatitudeThreshold plugin and lightning-filter CLI (metoppv#1550)
  Adds land-mask option to threshold CLI (metoppv#1559)
  Move statsmodels in environment yml files (metoppv#1556)
  rearranged args (metoppv#1558)
  Update Code-Style-Guide.rst (metoppv#1554)
  Updating timezone cli call (metoppv#1551)
MoseleyS pushed a commit to MoseleyS/improver that referenced this pull request Aug 22, 2024
* Updated CLI call to match other CLIs

* moved time to start and ancil to end

* moved documentation

* Moved ancil arg into *args to avoid kwarg requirement

* tuple friendly

* tidied docstring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BoM review required PRs opened by non-BoM developers that require a BoM review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants