Refactor 24 trend new - #216
Conversation
|
Yes, this is much easier to review. I'll close the other PR in favor of this one. |
krzywon
left a comment
krzywon
left a comment
There was a problem hiding this comment.
This looks like it takes into multi-dimensional trends properly. Look at the one comment I made to be sure that doesn't affect functionality before merging.
| if type(self.contents) is str: | ||
| children = f"\n{header} {self.contents}" | ||
| else: | ||
| match self.contents: |
There was a problem hiding this comment.
This match isn't completely equivalent to the previous if/else block. If self.contents == "", the new check will give children == f"\n{header} {self.contents}", but gave an empty string before.
There was a problem hiding this comment.
I've just restored the old behaviour by adding a check for an empty string in the case. I'm not exactly sure which is the best, but that might be a decision to make later down the line.
![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.
jamescrake-merani
left a comment
jamescrake-merani
left a comment
There was a problem hiding this comment.
I added a few comments (forgot to submit the review so they didn't show up) but I think lets just get this merged in now, and we can deal with them later. They are very minor, and would probably require a bit of discussion.
| ] | ||
|
|
||
| custom_test_directory = 'custom_test' | ||
| xml_file = path.join(path.dirname(__file__), "trend_test_data", "xml_test_files", "cansas1d_notitle.xml") |
There was a problem hiding this comment.
Nowadays I prefer using importlib resources for this. Basically, it allows you to import the folder this test data folder as a regular Python module, and then read data from it.
See:
https://docs.python.org/3/library/importlib.resources.html#importlib.resources.path
But this is a very minor comment, as this implementation probably still works fine.
|
|
||
| for i, datum in enumerate(self.data): | ||
| if not isinstance(datum, SasData): | ||
| invalid_indices.append(i) |
There was a problem hiding this comment.
ty (a Python type checker) is complaining that this code is unreachable, probably because self.data is type hinted to be a list of SasData, so therefore datum would never not be SasData. However, I appreciate that because Python is not strictly typed, its of course not impossible that datum is not SasData. But I think that if we get into type checking variables to make sure they're the type they should be, this could expand to cover all of the fields of these classes, at which point there are already libraries (e.g. pydantic) that handle this.
| if type(self.contents) is str: | ||
| children = f"\n{header} {self.contents}" | ||
| else: | ||
| match self.contents: |
There was a problem hiding this comment.
I've just restored the old behaviour by adding a check for an empty string in the case. I'm not exactly sure which is the best, but that might be a decision to make later down the line.
… previous refactor_24_trend branch
d78f578 to
ef38d6b
Compare
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.
|
I've just rebased to resolve the merge conflict. I'll check at the technical meeting, but if there are no objections then I will merge this PR. |
3 participants
New trend branch created directly from refactor_24. It contains the changes done in March to the original Trend class to allow defining an arbitrary number of axes in the Trend (populated either from the metadata or by giving a list of numerical values) and new tests, as well as some improvements to address the review comments in the previous refactor_24_trend branch.