-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix CI #47
Conversation
Reviewer's Guide by SourceryThis PR focuses on improving code quality and CI configuration through several key changes: updating the Ruff linter configuration, fixing test failures, adding type hints, and modernizing package dependencies. The changes are primarily maintenance-focused rather than introducing new features. Updated class diagram for SpectrogramCubeclassDiagram
class SpectrogramCube {
+__init__(data, wcs, mask, copy) : None
+__getitem__(item)
+__repr__() : str
+__str__() : str
+plot(*args, **kwargs)
}
Updated class diagram for SGMetaclassDiagram
class SGMeta {
+__init__(header, spectral_window, **kwargs) : None
+__str__() : str
+__repr__() : str
+_construct_time(key)
}
Updated class diagram for ObsIDclassDiagram
class ObsID {
+__init__(obsid) : None
+__repr__() : str
+_read_obsid(obsid)
}
Updated class diagram for SJIclassDiagram
class SJI {
+__init__(data, wcs, mask, copy, scaled, **kwargs) : None
+__repr__() : str
+__str__() : str
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nabobalis - I've reviewed your changes - here's some feedback:
Overall Comments:
- The dependency on
ndcube @ git+https://github.com/sunpy/ndcube@YOLO
is not suitable for a release. Please use a proper release version or at minimum a specific commit hash rather than an unstable branch name.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
"Topic :: Scientific/Engineering :: Physics", | ||
] | ||
dependencies = [ | ||
'dkist>=1.0.0', | ||
'ndcube>=2.1.2', | ||
'ndcube @ git+https://github.com/sunpy/ndcube@YOLO', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Using an unstable git branch reference for a production dependency is risky
Consider using a stable version or at minimum pinning to a specific commit hash for reproducible builds.
@pytest.mark.xfail | ||
def test_get_iris_response_version3(iris_response_v3): | ||
np_test.assert_almost_equal(iris_response_v3["AREA_SG"].value, area_sg_load3, decimal=6) | ||
np_test.assert_almost_equal(iris_response_v3["AREA_SJI"].value, area_sji_load3, decimal=6) | ||
np_test.assert_almost_equal(iris_response_v3["AREA_SG"].value, area_sg_load3) | ||
np_test.assert_almost_equal(iris_response_v3["AREA_SJI"].value, area_sji_load3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (testing): Missing explanation for expected test failure
The test is marked as expected to fail but there's no explanation why. Please add a reason parameter to the xfail decorator to document why this test is expected to fail. This helps other developers understand if this is a known limitation or a temporary issue that needs to be fixed.
np_test.assert_almost_equal(iris_response_v3["AREA_SG"].value, area_sg_load3) | ||
np_test.assert_almost_equal(iris_response_v3["AREA_SJI"].value, area_sji_load3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (testing): Decimal precision removed from assertions
The decimal precision parameter was removed from these assertions. This could make the tests more brittle as they now require exact matches. Consider keeping the decimal parameter to allow for small floating point differences.
Summary by Sourcery
Fix CI by updating linting rules and exclusions in ruff.toml, refactoring test files for consistency, and replacing pkg_resources with importlib.resources for resource management.
Enhancements:
CI:
Tests: