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

Fix CI issues and rubocop offenses #94

Closed
wants to merge 2 commits into from
Closed

Fix CI issues and rubocop offenses #94

wants to merge 2 commits into from

Conversation

syguer
Copy link

@syguer syguer commented Apr 21, 2023

Hi βœ‹

I've just fixed build errors and update Rubies to latest versions.
Also, fixed rubocop offenses using -A CLI option(Auto correct).
Enjoy 😎

P.S I'm ready to be gem owner for continuous maintenance. If you want, plz give me right on Rubygems! Thanks!


# This gem will work with 2.4.0 or greater... but *should* work with 1.8.7+
s.required_ruby_version = '>= 2.4.0'
s.required_ruby_version = '>= 2.7.0'
Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This depends on the version of ActiveModel installed.

https://github.com/rails/rails/blob/3-0-stable/activemodel/activemodel.gemspec

This gem requires Ruby 2.4.


jobs:
exclude:
# ref: https://rubies.travis-ci.org/index.txt
Copy link
Author

@syguer syguer Apr 21, 2023

Choose a reason for hiding this comment

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

Latest versions aren't supported for ppc64le.
ref: https://rubies.travis-ci.org/index.txt

Copy link
Member

Choose a reason for hiding this comment

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

It looks like they are. Where are you seeing that they are not supported?

TravisCI show all of the Rubies that we are testing. https://rubies.travis-ci.org/ They support Ruby 3.1.2 on ppc64le. I don't think that 3.2 was released when I last updated the tested Ruby versions. We should include those newer versions in testing.

I removed Ruby 2.4 form testing because it was having build issues on Travis. Though, IIRC, it would still build and test locally in my VMs.

@@ -12,15 +12,12 @@ Gem::Specification.new do |s|
README.md
CHANGELOG.md
]
s.files = `git ls-files -- lib/*`.split("\n")
Copy link
Author

Choose a reason for hiding this comment

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

This backquote section occurs build error on JRuby.

Copy link
Member

Choose a reason for hiding this comment

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

Is that a bug in JRuby? What does it cause a build error?

s.require_paths = %w[lib]

s.test_files = `git ls-files -- spec/*`.split("\n")
Copy link
Author

Choose a reason for hiding this comment

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

test_files is gone
ref: rubygems/guides#90

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this is correct. Please show me where test_files was removed. What commit?

@@ -2,8 +2,8 @@ Gem::Specification.new do |s|
s.name = 'email_validator'
s.version = '2.2.4'
Copy link
Author

Choose a reason for hiding this comment

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

[NOTICE]
You know, this version needs to bump up before gem push πŸ‘»

Copy link
Member

Choose a reason for hiding this comment

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

This is the current version of this gem.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why you are mentions this. Is there somewhere that I forgot up bump this? Did the automation fail somewhere? I don't understand why you are bringing this up.

@syguer
Copy link
Author

syguer commented Apr 24, 2023

@karlwilbur Hello πŸ˜„ Could you review this?

@karlwilbur
Copy link
Member

What build errors?

We should not be removing older Ruby versions from tests since we expect that this should run successfully on those older versions.

- 2.7.0
- 2.7.6
- 3.0.1
- 2.7.7
Copy link
Member

Choose a reason for hiding this comment

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

We need to keep these older Ruby versions. I want to ensure that we are testing in the versions that would be used in Rails 3 projects.

Copy link
Author

Choose a reason for hiding this comment

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

I want to ensure that we are testing in the versions that would be used in Rails 3 projects.

OK

@@ -16,7 +16,7 @@ AllCops:
Exclude:
- 'bin/*'
NewCops: enable
TargetRubyVersion: 2.4
TargetRubyVersion: 2.7
Copy link
Member

Choose a reason for hiding this comment

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

We need to keep the targeted version the same as the minimum version defined in our gemspec.

s.summary = 'An email validator for Rails 3+.'
s.description = 'An email validator for Rails 3+. See homepage for details: http://github.com/K-and-R/email_validator'
s.summary = 'An email validator for Rails.'
s.description = 'An email validator for Rails. See homepage for details: http://github.com/K-and-R/email_validator'
Copy link
Member

Choose a reason for hiding this comment

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

No. This is for "Rails 3+"

Copy link
Author

Choose a reason for hiding this comment

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

OK

@@ -126,7 +126,7 @@ class DefaultUserWithMessage < TestModel
"#{v}start-with-#{k}@valid-characters-in-local.dev"
]}).concat(valid_endable.map { |k, v| [
"end-with-#{k}-#{v}@valid-characters-in-local.dev"
]}).concat([
]}).push(
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok. I think we'll leave it unchanged for now.

Copy link
Author

Choose a reason for hiding this comment

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

OK

@syguer
Copy link
Author

syguer commented Apr 24, 2023

@karlwilbur Thank you for your review πŸ˜„
I didn't know You want to keep support Rails 3 so strongly... I'm so sorry.
My first motivation was to add CI against Ruby 3.2.x, so #95 looks good.
I'll close this PR and waiting for #95!
Thanks!

@syguer syguer closed this Apr 24, 2023
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