Repository navigation
Conversation
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
No quality gates enabled for this code.
See analysis details in CodeScene
Quality Gate Profile: Custom Configuration
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
jamescrake-merani
left a comment
jamescrake-merani
left a comment
There was a problem hiding this comment.
Thanks Adam, this I think is definitely an improvement. I just have a few comments I'd like you to consider before I approve.
| self, | ||
| value: QuantityType, | ||
| units: Unit, | ||
| units: NamedUnit, |
There was a problem hiding this comment.
Why has this been changed? My original understanding was that this was kept as Unit because the Quantity doesn't need to know the name of the Unit; it just needs to know details of the Unit to do the conversions.
There was a problem hiding this comment.
I just read your summary message again, and it looks like you did this to solve an error from a type checker. What was the error exactly?
There was a problem hiding this comment.
I've given this a second look and I can get around the typing issue without changing the Quantity class, so I've rolled back this change for now. The new correction can be seen in line 485 of metadata.py.
That said, there's part of me that does feel that Quantity should require a NamedUnit. From just a science perspective, if a quantity has a unit, but you can't tell me the unit, it feels pretty identical to their not being a unit. Still, I think that's a large debate that shouldn't be skipped.
| if result := known_mistake(unit_str): | ||
| return result | ||
| try: | ||
| import sasdata.quantities.units as units |
There was a problem hiding this comment.
Is there a reason a deferred import is needed here? If so, it might be good to add a comment explaining why.
There was a problem hiding this comment.
Good catch. I've moved the import back up to the top. I briefly considered just importing none from sasdata.quantities.units, but having both none and None be valid values in the file seemed like a disaster waiting to happen.
| @abc.abstractmethod | ||
| def from_json(cls, obj: dict) -> Self | None: | ||
| """Load this metadata from a JSON dict""" | ||
| return None |
There was a problem hiding this comment.
It might be better to raise a NotImplementedError for the abstract methods. This would mean that, in the case a subclass doesn't implement the method as it should, an error occurs rather than returning None.
| return str(x) | ||
|
|
||
| def _pretty_field(self, x: str) -> str: | ||
| match x: |
There was a problem hiding this comment.
It might be nicer to have a dictionary here instead of a match statement. I just did something similar today on the plotting refactor refactor (SasView/sasview#4166).
tab_name_dict = {0: "Data", 1: "Fit"}
def tab_name(i: int) -> str:
return tab_name_dict.get(i, "Unknown")There was a problem hiding this comment.
I get a little too happy with match statements. The dict is shorter.
This branch has not been deployed
2 participants
The primary goal of this PR is to provide an abstract base class unifying the
from_json,as_h5, andsummarymethods of the various Metadata component types, as mentioned in #244.This additionally unlocked the following other options:
introspect_summaryandsummarize_joinmethods that can now be shared between classes. This did necessitate small changes to the test files, since the different classes used to each have their own spacing convention, but are now all unified on four spaces and having the values aligned in the same columnmetadata.pyQuantityclass to use aNamedUnitinstead of aUnit. This solves one of the type issues.metadata.pyon the ruff formattingThe individual commits should all type-check, so I am happy to re-submit this PR as a subset of these features if it is too large on its own or any of these changes aren't desired.
Closes #244