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

Fix ignore functionality (especially for 'version' rules) #140

Merged
merged 17 commits into from
May 23, 2023

Conversation

CasperWA
Copy link
Collaborator

@CasperWA CasperWA commented Apr 18, 2023

Fixes #130

By extending the SemanticVersion class with rich comparison magic methods, the ignore rules for versions can be more easily checked.

An extended test ensures some more specific ignore rules are respected as intended.

Dependency version ranges are still not properly respected.
For example if >=1.0.0,<2 is specified, but the latest version is 2.3.1, then the result (with no ignore rules) will be >=2.3.1,<2, which makes no sense.
This should be remedied in another issue/PR.
Edit: This issue has now been reported in #141

@CasperWA CasperWA added the priority/high High priority issue/PR label Apr 18, 2023
@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #140 (0cb5694) into main (7907901) will increase coverage by 5.93%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##             main     #140      +/-   ##
==========================================
+ Coverage   65.67%   71.61%   +5.93%     
==========================================
  Files           9        9              
  Lines         472      546      +74     
==========================================
+ Hits          310      391      +81     
+ Misses        162      155       -7     
Impacted Files Coverage Δ
ci_cd/utils.py 94.16% <96.20%> (+24.39%) ⬆️
ci_cd/tasks/update_deps.py 81.31% <100.00%> (+0.21%) ⬆️

@ajeklund ajeklund self-requested a review May 11, 2023 13:30
Copy link
Contributor

@ajeklund ajeklund left a comment

Choose a reason for hiding this comment

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

This looks good although update_deps.py is (in my opinion) too long to understand and review efficiently. Perhaps it could be worth to clarify its central operational loop and move the logic functions into one or more separate files?

Also, Codecov generates a number of warnings about non-covered lines. Do these need to be addressed before merging?

pyproject.toml Outdated Show resolved Hide resolved
ci_cd/utils.py Outdated Show resolved Hide resolved
ci_cd/utils.py Outdated Show resolved Hide resolved
ci_cd/utils.py Outdated Show resolved Hide resolved
ci_cd/utils.py Outdated Show resolved Hide resolved
ci_cd/utils.py Outdated Show resolved Hide resolved
ci_cd/utils.py Outdated Show resolved Hide resolved
CasperWA and others added 2 commits May 12, 2023 16:08
Co-authored-by: Anders Eklund <96499163+ajeklund@users.noreply.github.com>
Co-authored-by: Anders Eklund <anders.eklund@sintef.no>
@CasperWA
Copy link
Collaborator Author

CasperWA commented May 12, 2023

This looks good although update_deps.py is (in my opinion) too long to understand and review efficiently. Perhaps it could be worth to clarify its central operational loop and move the logic functions into one or more separate files?

Completely fair.
I'll create an issue to follow-up on this, as I think it's out-of-scope for the current PR.

Edit: Opened #148

Also, Codecov generates a number of warnings about non-covered lines. Do these need to be addressed before merging?

Right. I'll have a look at the lot of them and see how easy it would be to check some of the non-covered lines.
The one that immediately sprung out at me was that the __hash__ function was not checked. Perhaps it can be removed altogether...

@CasperWA
Copy link
Collaborator Author

CasperWA commented May 23, 2023

I think I got most of the codecov messages now. Unfortunately, it seems codecov has not uploaded the checks for the latest commit. But as far as I know, there is only one left, which concerns a < resolve based on pre-release information. I think that's fine, to be honest.

@CasperWA CasperWA requested a review from ajeklund May 23, 2023 10:00
Copy link
Contributor

@ajeklund ajeklund left a comment

Choose a reason for hiding this comment

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

Excellent! Just one question added.

ci_cd/utils.py Show resolved Hide resolved
Minor is required to be present if patch is provided.
Patch is required if either pre_release or build are provided.
@CasperWA CasperWA requested a review from ajeklund May 23, 2023 12:15
Copy link
Contributor

@ajeklund ajeklund left a comment

Choose a reason for hiding this comment

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

Thank you!

@CasperWA CasperWA merged commit ebe8eef into main May 23, 2023
@CasperWA CasperWA deleted the cwa/fix-130-ignore-functionality branch May 23, 2023 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/high High priority issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with the ignore-functionality of ci-cd update-deps
2 participants