Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Remove underscores on suggest - updated for master #21

Merged
merged 5 commits into from

7 participants

@presidentbeef

Sorry, I don't see how to properly update the previous pull request.

The only difference here is that rubygems/rubygems@0e4b776 already took care of the unused variable.

Thanks.

@zenspider
Owner

Again. No. We've been over this.

The current suggestion system is good.

@presidentbeef

Sorry to be a bother, but I do not recall any discussion or rejection of this previously. Just for my own education, why is this not acceptable, given the performance and user experience improvements as shown in the pull request linked above?

@bowsersenior

@drbrain closed issue #19 in favor of this issue without commenting on the merit of either one:

Then this issue was closed by @zenspider with a cryptic reference to a previous discussion.

It seems like this pull request would improve the suggestions to me. What is the downside?

In any case, it would be much better if there was more detail on why the pull request was originally closed. That way, other developers can understand rubygems better and make better pull requests in the future. I think it's very important for the developers of open-source projects to go the extra mile to provide thoughtful feedback, document references to previous issues, and suggest ways to improve a pull request. Especially with a project as important and popular as rubygems!

@practicingruby
Collaborator

Folding on punctuation is pretty common in search/suggestion code, and this patch looks really simple to me. @zenspider: Can you give a specific reason why this should not be merged, or a reference to the old discussion?

/cc @bowsersenior @drbrain

@evanphx
Owner

While this seems stale, it might be more likely to be pulled if it included tests.

@bowsersenior

Seems like @sandal was cool with this without any tests since the change is so simple... Maybe one of the core committers could whip up a quick test and pull this thing in?

@presidentbeef

I can add a test, maybe later today. It will essentially be the same as the current tests for suggestions.

@practicingruby
Collaborator

@bowsersenior: That's not what my comment was meant to express. I was asking whether @zenspider had a specific reason for rejecting the patch, that's not to imply I thought the patch was ready to merge. That said, I think this is a good feature, so please add a test for it @presidentbeef.

@presidentbeef

Today, I again wished this were accepted.

Without patch:

$ time gem install coffeescript
ERROR:  Could not find a valid gem 'coffeescript' (>= 0) in any repository
ERROR:  Possible alternatives: coffee-script, coffee-cup, coffeeshop, coffee_cup, applescript

real  0m10.095s
user  0m9.443s
sys   0m0.037s

With patch:

$ time gem install coffeescript
ERROR:  Could not find a valid gem 'coffeescript' (>= 0) in any repository
ERROR:  Possible alternatives: coffee-script

real  0m1.644s
user  0m0.982s
sys   0m0.032s
@luislavena

Can you split @ui.error and check individual lines?

Asking because on Windows line endings are \r\n while POSIX is just \n, since you're comparing the exact output on this it might fail.

@luislavena
Collaborator

@evanphx seems test has been included, however I don't like the assert of it, commented above.

@travisbot

This pull request fails (merged 316ae3f into 294aa56).

@bowsersenior

Thanks @presidentbeef for your continuing dedication to this pull request over the past year. I would have given up by now!

@luislavena
Collaborator

@presidentbeef I'm testing this right now on Windows and will get back to you.

/cc @evanphx and @drbrain if no objection, will merge this to both master and 1.8 branch (since it is a performance regression on 1.8)

Thank you.

@zenspider
Owner

This is not a performance regression.

@evanphx evanphx merged commit 8e45010 into rubygems:master
@presidentbeef

Just noticed this was merged. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
10 lib/rubygems/spec_fetcher.rb
@@ -148,20 +148,18 @@ def spec_for_dependency(dependency, matching_platform=true)
end
##
- # Suggests a gem based on the supplied +gem_name+. Returns a string
- # of the gem name if an approximate match can be found or nil
- # otherwise. NOTE: for performance reasons only gems which exactly
- # match the first character of +gem_name+ are considered.
+ # Suggests gems based on the supplied +gem_name+. Returns an array of
+ # alternative gem names.
def suggest_gems_from_name gem_name
- gem_name = gem_name.downcase
+ gem_name = gem_name.downcase.tr('_-', '')
max = gem_name.size / 2
names = available_specs(:complete).first.values.flatten(1)
matches = names.map { |n|
next unless n.match_platform?
- distance = levenshtein_distance gem_name, n.name.downcase
+ distance = levenshtein_distance gem_name, n.name.downcase.tr('_-', '')
next if distance >= max
View
24 test/rubygems/test_gem_commands_install_command.rb
@@ -205,6 +205,30 @@ def test_execute_nonexistent_with_hint
assert_equal expected, @ui.error
end
+ def test_execute_nonexistent_with_dashes
+ misspelled = "non-existent_with-hint"
+ correctly_spelled = "nonexistent-with_hint"
+
+ util_setup_fake_fetcher
+ util_setup_spec_fetcher quick_spec(correctly_spelled, '2')
+
+ @cmd.options[:args] = [misspelled]
+
+ use_ui @ui do
+ e = assert_raises Gem::SystemExitException do
+ @cmd.execute
+ end
+
+ assert_equal 2, e.exit_code
+ end
+
+ expected = ["ERROR: Could not find a valid gem 'non-existent_with-hint' (>= 0) in any repository", "ERROR: Possible alternatives: nonexistent-with_hint"]
+
+ output = @ui.error.split "\n"
+
+ assert_equal expected, output
+ end
+
def test_execute_conflicting_install_options
@cmd.options[:user_install] = true
@cmd.options[:install_dir] = "whatever"
Something went wrong with that request. Please try again.