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.

Sign up for GitHub

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

Jump to bottom

fix(ruby): use more accurate versioning #1125

Merged
merged 1 commit into from
Dec 7, 2023
Merged

fix(ruby): use more accurate versioning #1125

merged 1 commit into from
Dec 7, 2023

Conversation

RTann
Copy link
Contributor

@RTann RTann commented Oct 26, 2023 •
edited
Loading

@RTann RTann requested a review from a team as a code owner October 26, 2023 20:10
@RTann RTann requested review from crozzy and hdonnay and removed request for a team October 26, 2023 20:10
@codecov
Copy link

codecov bot commented Oct 26, 2023 •
edited
Loading

Codecov Report

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

Comparison is base (53a666f) 52.06% compared to head (4779e1c) 52.39%.

❗ Current head 4779e1c differs from pull request most recent head 5b69353. Consider uploading reports for the commit 5b69353 to get more accurate results

Files Patch % Lines
ruby/version.go 95.08% 5 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1125      +/-   ##
==========================================
+ Coverage   52.06%   52.39%   +0.32%     
==========================================
  Files         220      221       +1     
  Lines       16843    16961     +118     
==========================================
+ Hits         8769     8886     +117     
- Misses       7264     7265       +1     
  Partials      810      810

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

RTann
RTann commented Oct 26, 2023
View reviewed changes
ruby/version.go Outdated Show resolved Hide resolved
@RTann RTann force-pushed the ruby-version-fix branch 5 times, most recently from 681454c to 52f72c8 Compare October 26, 2023 23:26
@RTann RTann requested review from jvdm and daynewlee October 26, 2023 23:29
@RTann RTann force-pushed the ruby-version-fix branch 6 times, most recently from 6d69a2e to 74854ac Compare October 30, 2023 20:38
crozzy
crozzy reviewed Nov 13, 2023
View reviewed changes
Copy link
Contributor

@crozzy crozzy left a comment

Choose a reason for hiding this comment

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

Thanks Ross, just a few comments

ruby/version.go Show resolved Hide resolved
ruby/version.go Outdated Show resolved Hide resolved
ruby/version.go Show resolved Hide resolved
ruby/version.go Show resolved Hide resolved
ruby/version.go Show resolved Hide resolved
@RTann RTann requested a review from crozzy November 14, 2023 00:30
@RTann
Copy link
Contributor Author

RTann commented Nov 14, 2023

@crozzy I decided to just not support Ruby versions with arbitrarily long integer sequences. Let me know if we should reconsider this, though.

@crozzy
Copy link
Contributor

crozzy commented Nov 16, 2023

@crozzy I decided to just not support Ruby versions with arbitrarily long integer sequences. Let me know if we should reconsider this, though.

IIUC the discussed solution was to compare padded strings?

@RTann
Copy link
Contributor Author

RTann commented Nov 17, 2023

@crozzy I decided to just not support Ruby versions with arbitrarily long integer sequences. Let me know if we should reconsider this, though.

IIUC the discussed solution was to compare padded strings?

Yep. Just added that now, if you can take a look

@RTann RTann force-pushed the ruby-version-fix branch 5 times, most recently from 1873da1 to e6bb3f9 Compare November 22, 2023 16:18
@RTann RTann force-pushed the ruby-version-fix branch 2 times, most recently from 0b13464 to 4779e1c Compare November 29, 2023 01:48
crozzy
crozzy approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@crozzy crozzy left a comment

Choose a reason for hiding this comment

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

LGTM

@RTann
Copy link
Contributor Author

RTann commented Dec 6, 2023

Rebased. Will merge with CI

@RTann RTann merged commit fbdad71 into quay:main Dec 7, 2023
7 checks passed
@RTann RTann deleted the ruby-version-fix branch December 7, 2023 21:59
@RTann RTann mentioned this pull request Jan 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crozzy crozzy crozzy approved these changes

@hdonnay hdonnay Awaiting requested review from hdonnay

@jvdm jvdm Awaiting requested review from jvdm

@daynewlee daynewlee Awaiting requested review from daynewlee

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RTann @crozzy