Skip to content

allow for different args with conduit > 12#53

Merged
ssicard merged 11 commits intomainfrom
ss/args-by-version
Mar 4, 2025
Merged

allow for different args with conduit > 12#53
ssicard merged 11 commits intomainfrom
ss/args-by-version

Conversation

@ssicard
Copy link
Contributor

@ssicard ssicard commented Feb 26, 2025

Description

Add compatibility for conduit 0.12 and 0.13 to accommodate updates in flags. This will allow for future conduit upgrades.

Quick checks:

  • I have followed the Code Guidelines.
  • There is no other pull request for the same update/change.
  • I have written unit tests.
  • I have made sure that the PR is of reasonable size and can be easily reviewed.

@ssicard ssicard marked this pull request as ready for review February 26, 2025 15:30
@ssicard ssicard requested a review from a team as a code owner February 26, 2025 15:30
@ssicard ssicard force-pushed the ss/args-by-version branch 2 times, most recently from f5c8574 to 03a990d Compare March 3, 2025 17:58
@ssicard ssicard force-pushed the ss/args-by-version branch from 03a990d to 25c2657 Compare March 3, 2025 20:00

func (f *Flags) ForVersion(ver string) []string {
constraints := map[string]string{
"v011": "< 0.12.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these will be simpler defined using the tilde operator, e.g. ~0.11 will be the equivalent of >= 0.11, < 0.12 and then the equivalent for 0.12 as well

See https://github.com/Masterminds/semver?tab=readme-ov-file#tilde-range-comparisons-patch

sanitized, _ := strings.CutPrefix(ver, "v")
v, _ := semver.NewVersion(sanitized)

for key, rule := range constraints {
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nitpick, if you change the key to the be the constraint, map[semver.Constraint]func()[]string)

you can essentially loop over

for rule, fn := range ... {
   if rule.Check(ver) {
      return fn() 
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That strategy would throw a parse error, since I would still need to ignore the error that may be returned from NewConstraint. I could declare the constraints before making the map, but that wouldn't save lines

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea that is a good point, though the errs returned by parsing new version and constraints should be checked, anyway. Referring to NewVersion and NewConstraint, this will reduce the errors with typos in code.

You can setup the constraints in NewFlags and make them part of Flags and yea that will be a bit more work.

v, _ := semver.NewVersion(sanitized)

for key, rule := range constraints {
c, _ := semver.NewConstraint(rule)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
c, _ := semver.NewConstraint(rule)
c, err := semver.NewConstraint(rule)
if err != nil {
return nil, fmt.Errorf("failed to parse constraint %q: %w", rule, err)
}

@ssicard ssicard merged commit fdcb9fe into main Mar 4, 2025
4 checks passed
@ssicard ssicard deleted the ss/args-by-version branch March 4, 2025 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants