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

Creation commands #82

Merged
merged 9 commits into from
Sep 25, 2020
Merged

Conversation

davidhassell
Copy link
Contributor

Transfer creation_commands methods from cf-python to cfdm, as mentioned in #53

@davidhassell
Copy link
Contributor Author

(I know that there are still a few TODOs in the docstrings ...)

@sadielbartholomew
Copy link
Member

About to add my review. Just going to open-close to trigger the CI.

@codecov
Copy link

codecov bot commented Sep 25, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@781a165). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #82   +/-   ##
=========================================
  Coverage          ?   87.30%           
=========================================
  Files             ?      101           
  Lines             ?     9958           
  Branches          ?        0           
=========================================
  Hits              ?     8693           
  Misses            ?     1265           
  Partials          ?        0           
Flag Coverage Δ
#unittests 87.30% <0.00%> (?)

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


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 781a165...951ba8b. Read the comment docs.

Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Two minor comments, but otherwise all is good. I've tested this end-to-end with cf-python by checking out also the creation-commands twin branch, as well as the cfdm standalone test suite, and the test suites both pass locally. I also tried out the new tutorial & API reference examples as a script and they work as they should.

I can't see any issue in the transfer of the relevant documentation, except for one word which should be amended (see sub-comment).

docs/source/tutorial.rst Show resolved Hide resolved
cfdm/test/test_Field.py Outdated Show resolved Hide resolved
@davidhassell
Copy link
Contributor Author

Thanks, @sadielbartholomew for the careful review. Does the latest commit sort all of the points you highlighted? If so, please merge! otherwise, let me know ...

@sadielbartholomew
Copy link
Member

sadielbartholomew commented Sep 25, 2020

Feedback has all been addressed, thanks @davidhassell.

I have just run the local test suites (cfdm & e2e cf-python) as a final check. Should strictly run the CI but I saw that the PEP8 test is failing the cfdm suite on a small number of cases, so that will fail or cancel the CI jobs anyway. I can do a commit after this to fix those PEP8 code violations.

This is good to merge.

@sadielbartholomew sadielbartholomew merged commit 2d99270 into NCAS-CMS:master Sep 25, 2020
@davidhassell
Copy link
Contributor Author

Thank you.

@sadielbartholomew
Copy link
Member

Oh, please ignore my comment above about PEP8 failures, I have just seen that they were all arising from a temporary script I specifically made to test which was this living under the repo. So the CI should pass - we can check from the result produced by my merge commit. 👍

@davidhassell davidhassell deleted the creation_commands branch November 14, 2022 09:14
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.

2 participants