Skip to content

Force all additional_attributes to str, since CMR only accepts str - #50

Closed
Cameronsplaze wants to merge 1 commit into
mainfrom
cjs/additional-attributes-str-default
Closed

Force all additional_attributes to str, since CMR only accepts str#50
Cameronsplaze wants to merge 1 commit into
mainfrom
cjs/additional-attributes-str-default

Conversation

@Cameronsplaze

@Cameronsplaze Cameronsplaze commented Mar 27, 2026

Copy link
Copy Markdown
Member

Pull Request Checklist

I have:

  • performed a self review of my code I&A code style
    • Resources and Data Structures are sorted by ABC or a defined sorting pattern
  • updated the documentation accordingly
  • verified required action checks are passing
  • bumped the version number as appropriate

@Cameronsplaze

Copy link
Copy Markdown
Member Author

Is there a reason to not force everything to be strings for additional attributes? It's bit me where I try to pass in a "int frame_number", and forgot to cast it to a string first. Plus it's slightly more readable with int(frame_num) vs str(int(frame_num))

I see an argument to "fail-fast" if you pass in a datetime object, and forget to string-ify first, so I'm debating if this is worth it too. However you should be inspecting the UMMG output anyways, and notice it's in a different format than what you'd expect.

Figured this was an easy PR to just get feedback on. Thoughts?

@reweeden

Copy link
Copy Markdown
Contributor

Well the type annotations of the additional_attribute factory function already require the values passed in to be str. So if you try to do something like additional_attribute("FOO", int(frame_number)) you should see a type checking error from a tool like mypy or ty.

I'm not sure if auto converting is the best choice here since it breaks the pattern that these factory functions follow which is to require their arguments to already be of the correct type using type signatures.

I would think as a first step it would be better to set up automatic enforced type checking through github actions on our CIRRUS repos. Or perhaps less ambitiously, set it up in your developer environment so that you get these warnings in your editor.

image

@reweeden

Copy link
Copy Markdown
Contributor

Also note that the get_additional_attribute method which is local to the cirrus umm-g classes DOES do forced string conversion which is fine since it's scoped to a much more specific problem. Within CIRRUS it would be fine to create whatever helpers you want, but I think for the core mandible library taking more of a 'soft enforcement' approach through type annotations over an 'autofix' approach is preferred.

@Cameronsplaze

Copy link
Copy Markdown
Member Author

For anyone else that finds this, we met up. Among other things, we setup mypy and it works great!

it would be better to set up automatic enforced type checking through github actions on our CIRRUS repos.

Oh I like the idea! My only worry is like the UmmgBase __init__ false-positive we saw, it might take a substantial amount of work to get the action passing. Although there might be an argument that moving linting actions to CIRRUS-Core might be beneficial for everyone?

But that's definitely outside of the scope of this issue on mandible.

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