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

Notes tags registration #35906

Merged
merged 1 commit into from Apr 15, 2019

Conversation

Projects
None yet
3 participants
@yoones
Copy link
Contributor

commented Apr 9, 2019

Summary

This PR adds to SourceAnnotationExtractor::Annotation a new method register_tags following the example of register_directories. It does not change the behavior of rake notes and allows tags registration through configuration.

@rails-bot rails-bot bot added docs railties labels Apr 9, 2019

@rafaelfranca
Copy link
Member

left a comment

Can you squash your commits?

@yoones yoones force-pushed the yoones:notes-tags-registration branch from df37c32 to 125fd3b Apr 12, 2019

@yoones yoones force-pushed the yoones:notes-tags-registration branch from 125fd3b to 6463cc0 Apr 12, 2019

@yoones

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@rafaelfranca I rebased on version 6.0.0.beta3 then squashed my commits.

@rafaelfranca rafaelfranca merged commit 81c8f18 into rails:master Apr 15, 2019

2 checks passed

buildkite/rails Build #60348 passed (10 minutes, 53 seconds)
Details
codeclimate All good!
Details
@@ -29,6 +29,16 @@ def self.register_directories(*dirs)
directories.push(*dirs)
end

def self.tags

This comment has been minimized.

Copy link
@sharang-d

sharang-d Apr 16, 2019

Contributor

Question: Does this not need to be documented(a small comment above the method name)?
Or do we not do it for methods who have self-explanatory names?

This comment has been minimized.

Copy link
@yoones

yoones Apr 16, 2019

Author Contributor

I chose to stay consistent with the rest of the code (neither self.directories nor self.extensions have comments above). Also I think this really does not need any explanation for we would just end up explaining memoization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.