-
Notifications
You must be signed in to change notification settings - Fork 553
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
2 changed files
with
10 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
class Thor | ||
VERSION = "0.17.0" | ||
VERSION = "0.18.0" | ||
end |
08265a3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This release broke the railties tests with Ruby 2.0 https://travis-ci.org/rails/rails/jobs/5811378#L589
08265a3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran tests on Ruby 1.9.3 before releasing but didn't think to run them again on Ruby 2.0.0? Have you bisected to determine specifically which commit is broke things?
08265a3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm running the tests right now. I'll bisect and send and report back later
08265a3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! If you can find the problem, I'm happy to push 0.18.1 later today.
08265a3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could track down the commit that changed the behavior. https://github.com/thor/thor/commit/fb149e3dd8b71124d194cee81e5f5f1f788be6c5
But I think it is a Rails fault, it is defining a protected
attr_reader
tofile_name
, but is using in a template name that method.I think that the methods used in template names should be public right?
08265a3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea why they’re protected? Looks like that was added by @drogus in rails/rails@0134c5c.
08265a3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sferik I don't think that there was any particular reason to make
file_name
protected, I was just following what was already there.@rafaelfranca I'm not sure what's the difference in using the method in a template name and in the template itself, shouldn't both cases use
NamedBase
as a context?08265a3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method is protected because if it is public, it will be run every time as a task, although for file_name, it is not really an issue.
In any case, I think the implementation in this commit fb149e3 is not good at all. I am skeptical all ruby implementations would have
attr_*
as the first line in the caller in there.I think what Rails is doing is fine. The real bug is here: d02f275
When rendering files, Thor behaves like Sinatra. It works as if the file was embedded in the current Thor class. Therefore, restricting the filename to be just public methods is a silly limitation that doesn't work well with how Thor is meant to be used.
08265a3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for not limiting the filename to be public methods.
08265a3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have opened a pull request that reverts these two regressions: #316