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

Add custom UnitRegistry to handle scaling factors more cleanly #212

Merged
merged 2 commits into from
Apr 2, 2024

Conversation

kroenlein
Copy link
Collaborator

An issue happened where stringified versions of parsed units weren't directly parsable on later invocations of the library when there was a scaling factor. For example, grams / 10 minutes was being internally represented as gram / _10_minute, but then the units service was storing that instead of what would have come out of a clean formatted string. The specific bug encountered is here:

https://citrine.atlassian.net/browse/SPT-1311

This PR prevents this issue in the future by:

  • Subclassing Unit so that it's __format__ method always cleans up the munged scaling factor
  • Deprecating clean since the subclass makes it obsolete
  • Adding testing

btreecat
btreecat previously approved these changes Apr 2, 2024
@@ -201,7 +207,58 @@ def _scaling_preprocessor(input_string: str) -> str:
return _scaling_store_and_mangle(input_string, todo)


_REGISTRY: UnitRegistry = None # global requires it be defined in this scope
def _unmangle_scaling(input_string: str) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

😆

assert "1e-5" in parse_units("F* mm*10**-5")
assert "1e" not in parse_units("F* kg * 10 cm")
assert "-3.07e2" in parse_units("F* -3.07 * 10 ** 2")
assert "11e2" in parse_units("F* 11*10^2")


def test_deprecation():
Copy link
Contributor

Choose a reason for hiding this comment

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

Best practice assume 3p libs are working



@functools.lru_cache(maxsize=1024)
def parse_units(units: Union[str, Unit, None],
def parse_units(units: Union[str, _ScaleFactorUnit, None],

Choose a reason for hiding this comment

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

does this mean I can't pass in a base Unit instance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It'd work, but typehints would complain. Functionally there's no reason to not support both, so I'll update that.

@@ -244,38 +301,23 @@ def convert_units(value: float, starting_unit: str, final_unit: str) -> float:


@register_unit_format("clean")
@deprecated(deprecated_in="2.1.0", removed_in="3.0.0", details="Scaling factor clean-up ")

Choose a reason for hiding this comment

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

if this is deprecated, what is it replaced with?

Choose a reason for hiding this comment

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

is it that _unmangle_scaling encompasses the customization that clean was performing before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the clean formatter is now obsolete, because of the __format__ method does its fundamental work without getting in the way of invoking other formatters (H, P, etc.). That's potentially very valuable because we'll because to do server-side HTML formatting, rather than forcing a browser side fix.

@kroenlein kroenlein merged commit ce04668 into main Apr 2, 2024
3 checks passed
@kroenlein kroenlein deleted the bugfix/spt-1311-accept-scaling-with-underscore branch April 2, 2024 19:43
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.

3 participants