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

Fix bug that caused cf.Field.del_file_location to fail when updating its metdata constructs #708

Merged
merged 3 commits into from
Jan 17, 2024

Conversation

davidhassell
Copy link
Collaborator

@davidhassell davidhassell commented Jan 11, 2024

Fixes #707

Also improves method order in API 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.

I've raised one question and one minor suggestion, but this solves the issue at hand and adds new relevant testing so all good and I am happy for this to be merged once those have been considered. Thanks.

Changelog.rst Outdated Show resolved Hide resolved
@@ -442,13 +442,13 @@ CFA
:toctree: ../method/
:template: method.rst

~cf.Field.file_locations
Copy link
Member

Choose a reason for hiding this comment

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

What (rough) ordering rules are we going for with respect to the API reference listings, e.g. here, for future reference (so I know what to try to abide by when adding new items myself, etc.)? It seemed to be alphabetical before but it appears that you are now preferring some loose form of categorisation based on the form of functionality?

Copy link
Collaborator Author

@davidhassell davidhassell Jan 17, 2024

Choose a reason for hiding this comment

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

Hi Sadie - anarchy rules! Sometimes it seems that alphabetical makes sense, other times some logical grouping seems appropriate. When (no if!) we refactor the whole documentation we'll look at this in the round.

Copy link
Member

Choose a reason for hiding this comment

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

anarchy rules

I have that on a T-shirt, with an exclamation at the front (not really, but it would work 😄 )!

Fair enough (when AI takes over it won't be able to automate such rules, so good thinking 😁 ). In that case, I'll generally not touch anything myself as to change the order (not that I had plans to, this would only be when otherwise touching the API reference method or class listings) unless it looks obviously out of alphabetical or (some) grouping order. Thanks for clarifying.

Co-authored-by: Sadie L. Bartholomew <[email protected]>
@davidhassell davidhassell merged commit 1571226 into NCAS-CMS:main Jan 17, 2024
@davidhassell davidhassell added bug Something isn't working CFA Relating to CFA datasets labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CFA Relating to CFA datasets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cf.Field.del_file_location fails with TypeError
2 participants