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

Provide "Did you mean? task_name" suggestions if task was not found #29

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 51 additions & 1 deletion lib/rake/task_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,29 @@ def [](task_name, scopes=nil)
self.lookup(task_name, scopes) or
enhance_with_matching_rule(task_name) or
synthesize_file_task(task_name) or
fail "Don't know how to build task '#{task_name}'"
build_task_fail(task_name)
end

# If a task match wasn't found, try to offer suggestions.
# The task name must be at least 2 characters.
def build_task_fail(task_name)
msg = "Don't know how to build task '#{task_name}'"
threshold = (task_name.size * 0.3).ceil

matches = @tasks.keys.
map {|name| [levenshtein_distance(name.downcase, task_name), name] }.
select {|distance, _| distance <= threshold }.
sort.
map(&:last)

if matches.size > 0
msg += ".\nDid you mean one of these? #{matches.join(", ")}"
end

fail msg
end
private :build_task_fail

def synthesize_file_task(task_name) # :nodoc:
return nil unless File.exist?(task_name)
define_task(Rake::FileTask, task_name)
Expand Down Expand Up @@ -301,6 +321,36 @@ def get_description(task)
desc
end

# Return a value representing the "cost" of transforming str1 into str2
def levenshtein_distance(str1, str2)
Copy link
Member

Choose a reason for hiding this comment

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

We pass this method around, but I think it should be public in RubyGems, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is. We can just include Gem::Textand call #levenshtein_distance. I think you already know it had a bug, but maybe we shouldn't worry about it?

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, Rake seems to keep a safe distance from RubyGems (maybe for a reason).

This could introduce some circular dependencies by depending on Gem itself.. and I'm not seeing anywhere besides testtask that uses it.

One could argue this method should be in a separate gem that both can require, and other people would find it useful. If that is the case, why not move it to core?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've been thinking about it - providing levenshtein_distance as a stdlib of Ruby. Rubygems and Bundler already (and possibly Rake) use the Levenshtein algorithm so I think it's a good idea to move it to core. I'll file a feature request to https://bugs.ruby-lang.org.

That being said, I realized that rake still needs to have support for 1.8.7, meaning we can't just use fresh features introduced recently. Should we just not merge this for now? I don't feel comfortable forcing others to maintain code that is copy-and-pasted...

Copy link
Member

Choose a reason for hiding this comment

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

We should avoid duplication whenever possible

Copy link
Member 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.

I don't see why we can't merge this now while we wait for ruby to have levenshtein distance.

Copy link
Member

Choose a reason for hiding this comment

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

If we merge this now I think we need to consider using Gem::Text instead of copy&paste

Copy link
Member

Choose a reason for hiding this comment

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

The necessary methods for Gem::Text don't exist for all versions of RubyGems that rake supports.

n = str1.length
m = str2.length
return m if n.zero?
return n if m.zero?

d = (0..m).to_a
x = nil

str1.each_char.each_with_index do |char1,i|
e = i+1

str2.each_char.each_with_index do |char2,j|
cost = (char1 == char2) ? 0 : 1
x = [
d[j+1] + 1, # insertion
e + 1, # deletion
d[j] + cost # substitution
].min
d[j] = e
e = x
end

d[m] = x
end

x
end

class << self
attr_accessor :record_task_metadata # :nodoc:
TaskManager.record_task_metadata = false
Expand Down
8 changes: 8 additions & 0 deletions test/test_rake_task.rb
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,14 @@ def test_find
assert_equal "Don't know how to build task 'leaves'", ex.message
end

def test_find_with_alts
task :tfind
ex = Task[:tfindx] rescue $!

assert_match /Don\'t know how to build task \'tfindx\'/, ex.message
assert_match /Did you mean one of these\? tfind/, ex.message
end

def test_defined
assert ! Task.task_defined?(:a)
task :a
Expand Down