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

feat: use semver to match required version #6066

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

ologbonowiwi
Copy link

  • test: add test for required_version
  • chore: install semver
  • refactor: use semver to compare versions
  • fix: use exact version for comparison when no comparator is specified
  • refactor: avoid overhead by considering default behavior from semver
  • test: add complex comparisons for required_version
  • refactor: handle required_version parsing manually

Closes #6063

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

Thanks for working on this one so soon. I'll be a little busy this coming weekend so I wanted to at least get my initial review done now. Good stuff so far!

src/config/mod.rs Outdated Show resolved Hide resolved
src/config/mod.rs Outdated Show resolved Hide resolved
src/config/mod.rs Show resolved Hide resolved
Configurations.md Outdated Show resolved Hide resolved
@ologbonowiwi
Copy link
Author

changes addressed @ytmimi. Thanks for the quick feedback 😄

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

A few more notes, but we're moving in the right direction

Configurations.md Outdated Show resolved Hide resolved
Configurations.md Outdated Show resolved Hide resolved
Configurations.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

I'll re-review this one again soon, just wanted to leave one quick comment on the updated docs, which I think are very user friendly!

When you have a moment could you squash all the commits down into a single commit.

Configurations.md Outdated Show resolved Hide resolved
Co-authored-by: Yacin Tmimi <[email protected]>
Configurations.md Outdated Show resolved Hide resolved
Comment on lines +232 to +240
required.split(',').enumerate().for_each(|(i, required)| {
let Some(comparator) = required_version.comparators.get_mut(i) else {
return;
};

if !required.contains('^') && comparator.op == semver::Op::Caret {
comparator.op = semver::Op::Exact;
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment somewhere to this section to explain that this is used to workaround the default behavior of the semver crate. I think having that context captured in a comment for future readers of the code would be very useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the check change to this?

-        if !required.contains('^') && comparator.op == semver::Op::Caret {
+        if !required.starts_with'^') && comparator.op == semver::Op::Caret {
            comparator.op = semver::Op::Exact;
        }

also, to avoid any confusion between the required passed to the check_semver_version and the required in the body of the closure maybe one of those variables should be renamed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use semver to check required_version instead of comparing two strings with !=
3 participants