A couple of small improvements

[mivanek]

Here are some improvements that I've been working on today. Still haven't implemented the -e flag, but I'm gonna work on that tomorrow, it's been a busy weekend.

Improvements in this commit:

• Fix a bug where adding a gem wouldn't fail when there is an already exiting gem in the Gemfile that doesn't have a version specified
• Add an --optimistic and --pessimistic flags that set the version constraints appropriately (credit to @moonglum for suggesting these flags)
• Some minor code refactoring

I'm thinking that maybe I could have included -o and -p flags as short versions of --optimistic and --pessimistic flags as well, but I'd like some input from you about that. If you think that's the way the go, I can easily make that change to this pull request right away.

Hope you'll like the improvements. :metal:

[DruRly]

Thanks for the pull @mivanek! I noticed a couple of issues while testing, one of which will be filed as a separate issue and will not hold up this merge.

The relevant issue is that when a gem, without a version number, is detected and the user provides the y confirmation to update; the gem gets declared twice in the Gemfile.

Replication:

1. Start out with a Gemfile which has turbolinks without a version number
2. gemrat turbolinks
3. Confirm the update with y
4. The Gemfile now contains:

...
gem 'turbolinks', '1.2.0'
gem 'turbolinks', '1.2.0'
...

Note: I think this can only be replicated if you do not have the latest version of a gem installed. Meaning that turbolinks did not declare a version number in the Gemfile but did not have the latest version in the Gemfile.lock. This triggered an update confirmation which resulted in the gem being declared twice. If you have the latest version, gemrat will just skip the confirmation and you will not be able to replicate this.

The other issue, which will be filed separately is installing sqlite3 when it already exists in the Gemfile. It does not detect that the gem already exists and adds gem 'sqlite3', '31.3.78632863260' to the Gemfile, which is not a valid version number. This is separate from the work in this merge and will be filed as a separate issue.

BTW I love the idea of having -o and -p since there is no .gemratrc file yet. It means that users will not be forced to pass long option names each time they want to use pessimistic or optimistic versioning. These options work great man! Thanks again :thumbsup:

[moonglum]

Super awesome :+1: Thanks for implementing my flag idea – currently a little busy, would've taken me some time to get there :wink:

[mivanek]

@DruRly Oops, looks like I accidentally closed this pull request, my bad. :smile: Reopened now. Anyway, I've just added -o and -p flags and pushed them here. I've also added tests to for -o and -p cases, but didn't cover the --no-version and -o/-p flag conflict since those tests are unnecessary because of the way the flags are implemented.

Concerning the bug you reported, I've dabbled a little but couldn't replicate the bug for now. I'll have more time later today, so I'll take a closer look at it then.

@moonglum I guess I owe you an apology for not checking if you have maybe started implementing those flags yourself, it completely slipped from my mind yesterday, so really sorry about that. I guess it turned out good in the end, but that's no excuse. I'm glad you like the changes, and thanks for the great idea. :wink:

[moonglum]

@mivanek No apology needed :wink: Happy that you took it and ran with it :+1:

[mivanek]

@DruRly I've tried replicating your bug, but it seems I'm not able to. I've tried replicating it with the hotfix branch from shime's repository, from shime's master branch and from your master branch and I can't replicate your results. Depending on the version, it either adds the gem 'turbolinks', '1.2.0', while ignoring gem 'turbolinks', and then failing the bundle (in case of your and shime's master branch) or it refuses to add to the gemfile (like it should with the hotfix branch). I've tried installing with gem 'turbolinks', '1.1.1' first, then removing the version number from the Gemfile, and then running bundle exec gemrat turbolinks, but it still either fails the bundle or refuses to add anything to the Gemfile. Honestly, I have no idea what's going on. I'm using ruby 1.9.3-p362, I've also tried it (by accident :smile: ) with ruby 2.0.0 and it's still acting the same. Do you have some other ideas what could be done to replicate this issue? Have I maybe misunderstood something?

[DruRly]

You've performed the replication steps and then some. Maybe I'm going crazy over here :smile: I'll try it again and if there's no issue, merge. Thanks for looking into this @mivanek!

[DruRly]

This seems to be isolated to turbolinks because gemrat skips jquery-rails which does not have a version number in the Gemfile either. jquery-rails does, however, have a version specified in Gemfile.lock.

If no one has an idea as to what's going on, I'm going to accept this merge because the bug seems to be isolated to me or edge cases. I'll wait for a response before I merge.

turbolinks details below:

Starting Gemfile Starting Gemfile.lock

$ gemrat turbolinks

Gem 'turbolinks' already exists, but there is a newer version of the gem (v 1.2.0 > v ..).
Update version? (Y/n) Y
Updated 'turbolinks' to version '1.2.0'.
Bundling...

$ vim Gemfile

Resulting Gemfile

$ git diff Gemfile.lock 

diff --git a/Gemfile.lock b/Gemfile.lock
index c362f19..7ea104a 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -116,5 +116,5 @@ DEPENDENCIES
   sass-rails (~> 4.0.0)
   sdoc
   sqlite3
-  turbolinks
+  turbolinks (= 1.2.0)
   uglifier (>= 1.3.0)

$ ruby -v
ruby 2.0.0p195 (2013-05-14 revision 40734) [x86_64-darwin11.4.2]

[mivanek]

Got it! I couldn't replicate the issue because of the difference in our Gemfiles since I don't use rails 4.0.0. The issue lies in the comment directly above the gem 'turbolinks' line, the way I've implemented parsing in this commit is not exclusive enough so it parsed the link to the github repository as an entry in the gemfile (that's why you got two entries to the gemfile - one for the comment and the other for the entry). This should be a relatively easy fix, I'll do some additional testing and maybe implement a new test case for covering this issue. I should probably be done and push the commit in 2 or 3 hours at the most.

[mivanek]

There, the bug should be fixed now. I realized that the get_current_gem_version method was actually picking up on a dot in github.com in the Gemfile and that dot got recognized as a version number. Since the current_gem_version variable would then be non-empty, it would fail the duplicate_gem? check and wouldn't raise an error. I've modified the reg exp for that check so it should only detect the version numbers now and ignore all the other dots in the Gemfile.

[DruRly]

Works great. Accepted! Thanks @mivanek. Those -o and -p options are pretty damn nice :beers:

[moonglum]

Awesome :racehorse:

[shime]

:beers: