arnau / ISO8601

Ruby parser to work with ISO8601 dateTimes and durations — http://en.wikipedia.org/wiki/ISO_8601
MIT License
75 stars 15 forks source link

Fix RuboCop offenses #60

Closed utkarsh2102 closed 4 years ago

utkarsh2102 commented 4 years ago

Hi @arnau :wave:

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

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

I hope this helps :100: I'll keep doing these small fixes as and when I keep getting time! Thanks for your awesome work :heart:

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

utkarsh2102 commented 4 years ago

Would you mind rebasing from master?

Should be done! \o/

arnau commented 4 years ago

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 commented 4 years ago

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 :rocket:

arnau commented 4 years ago

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

utkarsh2102 commented 4 years ago

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 commented 4 years ago

Yes I disabled CircleCI yesterday.

Perfect! :100:

Should be Travis only now, although it just failed. :thinking:

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

arnau commented 4 years ago
[!] 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 commented 4 years ago

Ah, got it! That should be an easy fix :sweat_smile:

I can do that in the next PR.

arnau commented 4 years ago

Thanks :)

arnau commented 4 years ago

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

utkarsh2102 commented 4 years ago

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! :smile: I can fix this in any way you want (though I prefer having a version.rb :sweat_smile:)

arnau commented 4 years ago

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