Refactor: QuantityFactory data dimensions operations

[FlorianDeconinck]

Description

The set_extra_dim_lengths function of QuantityFactory is badly named: it updates the dictionary of data dimensions, rather than set.

Usage of this function is also a bit odd, you mostly want to add dimensions. But because a QuantityFactory can be shared (should be shared) across many codes you want to make sure to not override previous dimensions set.

Introducing:

• QuantityFactory.add_data_dimensions: check for existing names and error out.
• QuantityFactory.update_data_dimensions: update existing or add new ones.

Deprecating set_extra_dim_lengths for later removal.

• Unit tests and proper type hints

How has this been tested?

New utest.

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation (e.g. add new modules to docs/docstrings/)
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules
• New check tests, if applicable, are included

[FlorianDeconinck]

Ping @CharlesKrop @katrinafandrich

[romanc]

I like the naming and I think we should propagate it down into the sizer.

[FlorianDeconinck]

Blocked by NOAA-GFDL/pace#154

[romanc]

That turned out bigger then I expected. Thanks for the cleanup! 🧹 ✨

[romanc]

It is a python anti-pattern to have an empty dict/list as default argument. The reason is that this will coupke function invocations because that empty dict might not be empty anymore once we get here in the next invocation. People usually use dict | None as type with None as default and the test for Non inside and assign a {} in that case (wich is then local to that call).

[romanc]

We can also do a follow-up, but basically, as far as I can see, the last commit, bcea590, wouldn't be necessary if we had

        data_dimensions: dict[str, int] | None = None,

in the argument list and then do some more fiddling inside the function. This is surprising, especially coming form C++ where default arguments behave differently. Here's a simple example of what goes wrong.

I think there's mypy or flake rules to catch those. I remember seeing them (probably in gt4py). I'll search tomorrow.

[romanc]

Ah it's in flake8-bugbear

B006: Do not use mutable data structures for argument defaults. They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them.

[romanc]

follow-up is here: #277

[FlorianDeconinck]

Another way to fix this is actually use a dataclasses.field(default_factory) on the base dataclass