Skip to content

Tweak warning recommendation#2266

Merged
bundlerbot merged 3 commits intoruby:masterfrom
deivid-rodriguez:refine_warning
Apr 24, 2018
Merged

Tweak warning recommendation#2266
bundlerbot merged 3 commits intoruby:masterfrom
deivid-rodriguez:refine_warning

Conversation

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez commented Apr 10, 2018

Description:

In #2242 I introduced a recommendation to fix the semver warning in the case where the dependency is not semantically versioned. However, I think the recommendation was incorrect since, for example, < 5.2 still allows 5.2.0.rc1, whereas something like ~> 5.1.5 does not.

I was bitten by this when I updated my dependencies according to the recommendation and the rails-5.2.0.rc1 gem installed on my system was unintentionally activated.

Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

It should be fully equivalent to the old one and not allow pre-releases.
add_#{dep.type}_dependency '#{dep.name}', '~> #{base.join '.'}', '>= #{dep_version}'
if #{dep.name} is not semantically versioned, you can bypass this warning with:
add_#{dep.type}_dependency '#{dep.name}', '>= #{dep_version}', '< #{upper_bound.join '.'}'
add_#{dep.type}_dependency '#{dep.name}', '>= #{dep_version}', '< #{upper_bound.join '.'}.x'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why .x as the upper bound? This will continue to allow #{upper_bound}.a to be resolved, I believe

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure why, but it works, I think I saw it somewhere. If I use < 5.2.x, it no longer allows 5.2.0.rc1 whereas < 5.2 allows it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is it because x is alphabetically before 0? 😂

Copy link
Copy Markdown
Contributor Author

@deivid-rodriguez deivid-rodriguez Apr 11, 2018

Choose a reason for hiding this comment

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

Funny I thought this was a feature and it seems like it's a bug 🤣.

If there's no "supported" equivalent of the semver operator using ">=", "<", then this advice is actually misleading...

Copy link
Copy Markdown
Contributor Author

@deivid-rodriguez deivid-rodriguez Apr 14, 2018

Choose a reason for hiding this comment

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

I only now understood @segiddins original comment. I'll do some more experiments to confirm if the actual non numeric digit is actually relevant. It seems better to use .a anyways since it's already used in other places.

Since it's documented at least in the test suite.
@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

deivid-rodriguez commented Apr 11, 2018

Turns out that .a is actually documented for usage with pre-releases (at least via tests). I've added a commit to switch to using .a and another one to add a test to be explicit about how .a and < behave together.

@bronzdoc
Copy link
Copy Markdown
Contributor

@bundlerbot r+

@bundlerbot
Copy link
Copy Markdown
Contributor

📌 Commit 83552ed has been approved by bronzdoc

@bundlerbot
Copy link
Copy Markdown
Contributor

⌛ Testing commit 83552ed with merge 47d6a7d...

bundlerbot added a commit that referenced this pull request Apr 24, 2018
Tweak warning recommendation

# Description:

In #2242 I introduced a recommendation to fix the semver warning in the case where the dependency is not semantically versioned. However, I think the recommendation was incorrect since, for example, `< 5.2` still allows `5.2.0.rc1`, whereas something like `~> 5.1.5` does not.

I was bitten by this when I updated my dependencies according to the recommendation and the `rails-5.2.0.rc1` gem installed on my system was unintentionally activated.

# Tasks:

- [x] Describe the problem / feature
- [x] Write tests
- [x] Write code to solve the problem
- [ ] Get code review from coworkers / friends

I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).
@bundlerbot
Copy link
Copy Markdown
Contributor

☀️ Test successful - status-travis
Approved by: bronzdoc
Pushing 47d6a7d to master...

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.

5 participants