-
Notifications
You must be signed in to change notification settings - Fork 0
[Refactor] Align structure of tests to be in line with recommendations from PyPa #110
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
Conversation
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.
Pull Request Overview
This PR refactors several modules to align the structure of tests and code with the recommendations from PyPa, removing unused imports and simplifying error message formatting. Key changes include:
- Removing unused imports and parameters across multiple files.
- Replacing f-string usage with literal strings where dynamic content is unnecessary.
- Refactoring file path handling in truncation rule files using a new helper method.
Reviewed Changes
Copilot reviewed 138 out of 138 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| aps/rms_jobs/updateAPSModelFromUncertaintyTable.py | Removed unused get_workflow_name import. |
| aps/rms_jobs/updateAPSModelFromFMU.py | Simplified print statement string formatting. |
| aps/rms_jobs/turn_off_traceback.py | Removed unused os import. |
| aps/rms_jobs/test_jobs_and_workflow.py | Eliminated several unused imports for clarity. |
| aps/rms_jobs/import_fields_from_disk_original.py | Changed formatting style for error message. |
| aps/rms_jobs/export_fmu_config_files.py | Adjusted print messaging to use literal strings. |
| aps/rms_jobs/export_fields_to_disk.py | Updated error messages to remove unnecessary f-string notation. |
| aps/rms_jobs/create_simulation_grid.py | Removed unused numpy import. |
| aps/rms_jobs/check_grid_index_origin.py | Standardized error message formatting. |
| aps/algorithms/truncation_rules/*.py | Updated conditional checks and added all lists for explicit exports. |
| aps/algorithms/defineTruncationRule.py | Refactored file path resolution using a helper method. |
| aps/algorithms/defineFacies.py | Removed redundant type import for Dict. |
| aps/algorithms/APSZoneModel.py, APSMainFaciesTable.py | Cleaned up unused imports. |
| aps/algorithms/APSGaussModel.py | Modified error messages to remove class name information. |
|
@sindre-nistad will review this once the conflicts are resolved :) |
4c90e9b to
0544c75
Compare
|
@einarwar; I've resolved the merge conflics |
cdd53e2 to
79095a1
Compare
einarwar
left a comment
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.
Solid work, good job! Seems like a good change, with more code reusability.
Im sorry for nitpicking on files that were mostly just renamed/moved. Also i am not very knowledgeable about this repo, so some comments might not be relevant.
One thing i would suggest is to add type annotations in the tests where fixtures are used.
8166e5d to
6839861
Compare
|
@einarwar I've resolved your suggestions and feedback. |
einarwar
left a comment
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.
LGTM. Once tests pass should be good to go :)
This makes it much easier to behave the same, independent of where it was invoked from
The tests are re-organized to that `pytest` can easily execute them The tests also use more features of `pytest` to reduce boilerplate
6839861 to
742ec26
Compare
|
@einarwar; I did another (force) push, which addresses the rest of your feedback |
Reasons for making this change
This should make the package easier to publish (ref. PyPa)
I've also simplified som of the testing to use
contest.pywhen appropriate.Related merge requests
Part of #109.
Builds on #108 (which should be merged before this)
Checklist