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

Utilize packaging.version.Version #220

Merged
merged 13 commits into from
Dec 7, 2023
Merged

Conversation

CasperWA
Copy link
Collaborator

@CasperWA CasperWA commented Nov 21, 2023

Fixes #221

Since we are currently only supporting Python packages, it makes sense to use the packaging.version.Version class to parse the latest retrieved version from PyPI. This ensures we can handle the quirks of Python versions.

Addresses the bold parts of #217, and the immediate bug there.

Specifically, this addresses the epoch and post-release segments, adding tests for these as well.

For now, all non base_version segments are stripped from the version, meaning only epoch (if present) + release (major.minor.patch) are included from a parsed Version instance.

Extra logic is needed to handle and support the remaining Version segments, as is also mentioned in #217.

Implementing digestion of packaging.version.Version instances in
SemanticVersion to be used when updating version specifiers, but still
have the usability of the special SemanticVersion methods.
Implement support for packaging.version.Version in SemanticVersion,
making it able to parse its current internals as a
packaging.version.Version, as well as being able to digest one.

Add epochs to versions if necessary.
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (fff08c4) 80.07% compared to head (32f856e) 80.85%.

Files Patch % Lines
ci_cd/tasks/update_deps.py 52.94% 8 Missing ⚠️
ci_cd/utils/versions.py 95.08% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #220      +/-   ##
==========================================
+ Coverage   80.07%   80.85%   +0.78%     
==========================================
  Files          12       12              
  Lines         838      888      +50     
==========================================
+ Hits          671      718      +47     
- Misses        167      170       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@CasperWA CasperWA marked this pull request as draft November 21, 2023 12:17
@CasperWA CasperWA marked this pull request as ready for review November 21, 2023 12:21
@CasperWA CasperWA added the bug Something isn't working label Nov 22, 2023
@daniel-sintef
Copy link
Contributor

Overall general comments:

  • Areas for Improvement:
    • Complexity and Readability: Some parts, especially the update_specifier_set function, exhibit complexity that could hinder maintainability. Breaking down complex logic into smaller functions is advisable.
    • Error Handling: The conversion of version strings to Version objects lacks error handling, which could lead to issues with invalid version strings.
    • Overgeneralized Patterns: The broad regex pattern might lead to unexpected string matches. A more precise pattern could reduce erroneous matches.
    • Epoch Handling: The logic surrounding epoch handling is somewhat convoluted and could be made clearer.
    • Testing Gaps: Certain new code paths, particularly those handling edge cases, lack test coverage.

Specific Suggestions:

  • Refactoring update_specifier_set: Split this function to manage its complexity. For example, handling different version segments (major, minor, patch, epoch) in separate functions could improve clarity.
  • Enhanced Error Handling: Implement robust error handling for the Version constructor to manage invalid version strings.
  • Regex Precision: Fine-tune the regex pattern used for version matching to avoid capturing unintended strings.
  • Clearer Epoch Logic: Revise the epoch handling logic for clarity. Perhaps introduce a dedicated method or clear inline documentation to explain the rationale behind the current approach.
  • Increased Test Coverage: Ensure new logic, particularly around edge cases and error handling, is adequately covered by tests.

Copy link
Contributor

@daniel-sintef daniel-sintef left a comment

Choose a reason for hiding this comment

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

I think this code probably introduces more complexity than it needs to, missing test coverage is also rather worrysome. I think some of the functions need to be refactored a bit mroe

@CasperWA
Copy link
Collaborator Author

CasperWA commented Nov 23, 2023

I think this code probably introduces more complexity than it needs to, missing test coverage is also rather worrysome. I think some of the functions need to be refactored a bit mroe

The missing test coverage I'd consider quite minimal here (see the codecov report comment above). However, yes, in general (meaning prior to this PR) the test coverage of parts in versions.py is bad, indeed.

Unfortunately, the whole thing is quite complex, and there's a lot of added complexity due to a lot of error handling.
The best way to ensure the complexity is valid is through unit testing, so I'd consider #211 of a higher priority going forward.

@CasperWA
Copy link
Collaborator Author

CasperWA commented Nov 23, 2023

Concerning the curated AI overview, I have addressed all of these points, except the first one: refactoring update_specifier_set(). This is indeed a function that should be refactored and tested thoroughly as it's at the core of the whole logic of the update_deps task.

Co-authored-by: Daniel Marchand <[email protected]>
@CasperWA
Copy link
Collaborator Author

Concerning the AI-generated overview, I have addressed all of these points, except the first one: refactoring update_specifier_set(). This is indeed a function that should be refactored and tested thoroughly as it's at the core of the whole logic of the update_deps task.

To follow up on this, I have opened #222.

@CasperWA CasperWA merged commit 7f11bdb into main Dec 7, 2023
24 checks passed
@CasperWA CasperWA deleted the cwa/fix-217-post-release-segment branch December 7, 2023 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support epoch and post version segments
2 participants