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

Support another file type .rake in rake notes #7871

Merged
merged 1 commit into from Oct 12, 2012
Merged

Support another file type .rake in rake notes #7871

merged 1 commit into from Oct 12, 2012

Conversation

bjnord
Copy link
Contributor

@bjnord bjnord commented Oct 8, 2012

Similar to commit 55ceced which added support for .css .scss and .js, this commit adds .rake files to the list of files scanned by the rake notes and rake notes:custom tasks. These tasks will now pick up FIXME and TODO in lib/tasks/*.rake custom rake tasks.

@rafaelfranca
Copy link
Member

Could you add a CHANGELOG entry?

@@ -33,8 +33,8 @@ def to_s(options={})

# Prints all annotations with tag +tag+ under the root directories +app+, +config+, +lib+,
# +script+, and +test+ (recursively). Only filenames with extension
Copy link
Member

Choose a reason for hiding this comment

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

"Only" sounds odd when nearly a dozen extensions are supported !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going for minimal change (only adding the new extension). I'm happy to commit a change to remove the word "Only" here and line 59 if you'd like.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah got that. Please go ahead and make the fix. Thanks :)

@steveklabnik
Copy link
Member

This will need rebased, and while you're at it, should get squashed.

@bjnord
Copy link
Contributor Author

bjnord commented Oct 12, 2012

The squashed commit 28b32df has all my changes. Hope I did the rebasing right (first time); if not, please let me know what I need to do differently. Thanks.

@steveklabnik
Copy link
Member

So, https://github.com/rails/rails/pull/7871/commits shows seven commits. Ideally, it should show just 28b32df. That said, it should be easy enough for @rafaelfranca or whoever merges to just cherry-pick that.

Normally, you'd do something like this. I'm assuming upstream is pointing at rails/rails.git:

$ git fetch upstream
$ git checkout master
$ git rebase upstream/master
$ git rebase -i

< choose squash for all of your commits>

$ git push origin master -f

Note that it's often good to make patches against a new branch, and not your master; this only matters if you submit two pulls at once, which can happen since pulls sit for a month or two at times. :)

@steveklabnik
Copy link
Member

(fwiw, it looks like you squashed fine, except the CHANGELOG entry isn't in 28b32df. Looks like the rebase didn't quite work out)

@bjnord
Copy link
Contributor Author

bjnord commented Oct 12, 2012

Steve: Thanks so much for the detailed explanation. You're right, I should have used a branch; will do that next time. I would have known how to squash a branch and move it forward as needed.

[See comment below; commit 06129c0 has them all now.]

@steveklabnik
Copy link
Member

Sorry! I messed up. Don't squash your first commit. the rest squash into it, make sense?

@bjnord
Copy link
Contributor Author

bjnord commented Oct 12, 2012

Finally I think I might have it: The commit 06129c0 has all the changes in a single commit, including the CHANGELOG.

@steveklabnik
Copy link
Member

:D looks like it...

rafaelfranca added a commit that referenced this pull request Oct 12, 2012
Support another file type .rake in `rake notes`
@rafaelfranca rafaelfranca merged commit d08b89f into rails:master Oct 12, 2012
@rafaelfranca
Copy link
Member

Thank you guys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants