Support another file type .rake in `rake notes` #7871

Merged
merged 1 commit into from Oct 12, 2012

Conversation

Projects
None yet
4 participants
@bjnord
Contributor

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

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Oct 8, 2012

Member

Could you add a CHANGELOG entry?

Member

rafaelfranca commented Oct 8, 2012

Could you add a CHANGELOG entry?

@vijaydev

View changes

railties/lib/rails/source_annotation_extractor.rb
@@ -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

This comment has been minimized.

Show comment Hide comment
@vijaydev

vijaydev Oct 10, 2012

Member

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

@vijaydev

vijaydev Oct 10, 2012

Member

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

This comment has been minimized.

Show comment Hide comment
@bjnord

bjnord Oct 10, 2012

Contributor

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.

@bjnord

bjnord Oct 10, 2012

Contributor

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.

This comment has been minimized.

Show comment Hide comment
@vijaydev

vijaydev Oct 10, 2012

Member

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

@vijaydev

vijaydev Oct 10, 2012

Member

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

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Oct 11, 2012

Member

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

Member

steveklabnik commented Oct 11, 2012

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

@bjnord

This comment has been minimized.

Show comment Hide comment
@bjnord

bjnord Oct 12, 2012

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Oct 12, 2012

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. :)

Member

steveklabnik commented Oct 12, 2012

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

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Oct 12, 2012

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)

Member

steveklabnik commented Oct 12, 2012

(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

This comment has been minimized.

Show comment Hide comment
@bjnord

bjnord Oct 12, 2012

Contributor

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.]

Contributor

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

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Oct 12, 2012

Member

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

Member

steveklabnik commented Oct 12, 2012

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

@bjnord

This comment has been minimized.

Show comment Hide comment
@bjnord

bjnord Oct 12, 2012

Contributor

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

Contributor

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

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Oct 12, 2012

Member

:D looks like it...

Member

steveklabnik commented Oct 12, 2012

:D looks like it...

rafaelfranca added a commit that referenced this pull request Oct 12, 2012

Merge pull request #7871 from bjnord/master
Support another file type .rake in `rake notes`

@rafaelfranca rafaelfranca merged commit d08b89f into rails:master Oct 12, 2012

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Oct 12, 2012

Member

Thank you guys

Member

rafaelfranca commented Oct 12, 2012

Thank you guys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment