-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add GitHub Actions CI and ruby 3+ support #68
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fantastic! Thanks for this great PR. I don't have too much background with squid, and just have a couple question about your changes.
squid.gemspec
Outdated
@@ -20,6 +20,9 @@ Gem::Specification.new do |spec| | |||
spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) } | |||
spec.require_paths = ['lib'] | |||
|
|||
if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('3.1.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe that rubygems supports conditional dependencies. Can you explain a bit what you're trying to achieve with this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't realize that. I've updated with an older minimum version that does not require ruby 2.5+.
As for the reason, the matrix
gem is no longer a default gem in 3.1+.
If you don't mind dropping support for ruby 2.4. https://rubygems.org/gems/matrix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your latest revision seems like a good solution to me. Thanks!
spec/squid_spec.rb
Outdated
@@ -6,7 +6,7 @@ | |||
let(:data) { {} } | |||
let(:options) { {legend: false, baseline: false, steps: 0, format: :currency} } | |||
let(:settings) { options } | |||
let(:blue_rgb) { [0.1804, 0.3412, 0.549] } | |||
let(:blue_rgb) { %w[2e 57 8c].map { |c| (c.hex / 255.0).round(4) } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this change doing? Can you explain how the behavior changes here? It seems like we could have a potential breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line in particular was an artifact of some debugging I was doing on this expectation:
colors_of
calls PDF inspector which introduces a little rounding error.
1) Prawn::Document#chart given one series has blue columns
Failure/Error: expect(colors_of(chart).fill_color).to eq blue_rgb
expected: [0.1804, 0.3412, 0.549]
got: [0.18039, 0.34118, 0.54902]
(compared using ==)
# ./spec/squid_spec.rb:44:in `block (3 levels) in <top (required)>'
In any case, I've reverted the line you've highlighted and the specs pass.
@justinhoward, thanks for getting back to me on this. Sorry, I just saw this a few minutes ago. I've responded to your comments and also bumped the workflow file to the latest minor ruby versions. Please let me know if I can be of further assistance. |
Looks great! Thanks so much 🎉 |
@claudiob Would you be able to add me as a gem owner? https://rubygems.org/profiles/justinhoward |
Hi,
I really appreciate this library. It's been very useful.
Here is a patch which:
You can see the test suite passing here: https://github.com/kfprimm/squid/actions/runs/3972382795
Thanks.