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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RuboCop offenses #60

Merged
merged 8 commits into from Jul 4, 2020
Merged

Fix RuboCop offenses #60

merged 8 commits into from Jul 4, 2020

Conversation

utkarsh2102
Copy link
Contributor

@utkarsh2102 utkarsh2102 commented Jul 3, 2020

Hi @arnau 馃憢

As discussed in the previous PR, here are a bunch of fix, each in a separate and a meaningful commit! 馃殌

In case you find them good, please don't squash them while merging (since each commit represents a different set of changes) 馃槃
(and this way, it's easier to troubleshoot/debug, if something goes wrong or need to be reverted!)

I hope this helps 馃挴
I'll keep doing these small fixes as and when I keep getting time!
Thanks for your awesome work 鉂わ笍

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>

Copy link
Owner

@arnau arnau left a comment

Choose a reason for hiding this comment

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

Looks great 馃憣

Would you mind rebasing from master? I'll merge after that.

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
@utkarsh2102
Copy link
Contributor Author

Would you mind rebasing from master?

Should be done! \o/

@arnau arnau merged commit d5a5be7 into arnau:master Jul 4, 2020
@utkarsh2102 utkarsh2102 deleted the fix-rubocop-offenses branch July 4, 2020 11:21
@arnau
Copy link
Owner

arnau commented Jul 4, 2020

Given that these changes change the minimum Ruby version, I'm thinking in bumping to 0.13.0. Any thoughts?

Also, any other changes you think would be good to bring to the next version?

@utkarsh2102
Copy link
Contributor Author

I was gonna suggest disabling CircleCI and enabling Travis (if that's disabled in favor of CircleCI?), but I think you already disabled CircleCI, right?

If not, can you do so!? I'd love to get the build green 馃殌

@arnau
Copy link
Owner

arnau commented Jul 4, 2020

Yes I disabled CircleCI yesterday. Should be Travis only now, although it just failed. 馃

@utkarsh2102
Copy link
Contributor Author

Before you bump the version, I have some more minor changes I want to bring.

  1. Make the CI green.
  2. Introduce a Rakefile --> run tests and rubocop on default. So that next set of PRs will run those tests and we'll know if something needs fixing, etc.
  3. <some more minor edits, as we go!>

@utkarsh2102
Copy link
Contributor Author

Yes I disabled CircleCI yesterday.

Perfect! 馃挴

Should be Travis only now, although it just failed. 馃

That's weird. I don't see any CI running (or failing..)?
cf: https://github.com/arnau/ISO8601/commits/master

@arnau
Copy link
Owner

arnau commented Jul 4, 2020

[!] There was an error parsing `Gemfile`:
[!] There was an error while loading `iso8601.gemspec`: cannot load such file -- /Users/arnau/kitchen/iso8601/iso8601/version
Does it try to require a relative path? That's been removed in Ruby 1.9. Bundler cannot continue.

 #  from /Users/arnau/kitchen/iso8601/iso8601.gemspec:3
 #  -------------------------------------------
 #
 >  require_relative 'iso8601/version'
 #
 #  -------------------------------------------
. Bundler cannot continue.

 #  from /Users/arnau/kitchen/iso8601/Gemfile:5
 #  -------------------------------------------
 #
 >  gemspec
 #  -------------------------------------------

https://travis-ci.org/github/arnau/ISO8601/jobs/704893061

@utkarsh2102
Copy link
Contributor Author

Ah, got it!
That should be an easy fix 馃槄

I can do that in the next PR.

@arnau
Copy link
Owner

arnau commented Jul 4, 2020

Thanks :)

@arnau
Copy link
Owner

arnau commented Jul 4, 2020

It might be better to remove the version.rb file and just use a string in gemfile.

@utkarsh2102
Copy link
Contributor Author

I think it's okay to use version.rb as well. That's mostly kind-of a default way now.
Even bundler does that when we create a new gem via bundle gem foo.

That said, it's totally up to you! 馃槃
I can fix this in any way you want (though I prefer having a version.rb 馃槄)

@arnau
Copy link
Owner

arnau commented Jul 4, 2020

Ok, if it's the default way, happy to keep it :)

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.

None yet

2 participants