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

Add Public Api for Register New Extensions for Rake Notes #14379

Merged
merged 8 commits into from Mar 17, 2014

Conversation

@robertomiranda
Copy link
Contributor

robertomiranda commented Mar 14, 2014

ref #14214

@guilleiguaran
guilleiguaran reviewed Mar 14, 2014
View changes
railties/lib/rails/source_annotation_extractor.rb Outdated

# Registers new Annotations File Extensions
# SourceAnnotationExtractor::Annotation.register_extensions("css", "scss", "sass", "less", "js") { |tag| /\/\/\s*(#{tag}):?\s*(.*)$/ }
def self.regiter_extensions(*extensions, &block)

This comment has been minimized.

Copy link
@guilleiguaran

guilleiguaran Mar 14, 2014

Member

typo: regiter => register

@guilleiguaran
Copy link
Member

guilleiguaran commented Mar 14, 2014

@carlosantoniodasilva
carlosantoniodasilva reviewed Mar 14, 2014
View changes
railties/lib/rails/source_annotation_extractor.rb Outdated
def self.directories
@@directories ||= %w(app config db lib test) + (ENV['SOURCE_ANNOTATION_DIRECTORIES'] || '').split(',')
end

def self.extensions
@@extensions ||= DEFAULT_EXTENSIONS

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Mar 14, 2014

Member

Maybe you don't need defaults, you can just use the register method to create all defaults, wdyt?

This comment has been minimized.

Copy link
@robertomiranda

robertomiranda Mar 14, 2014

Author Contributor

sounds good 👍

@carlosantoniodasilva
carlosantoniodasilva reviewed Mar 14, 2014
View changes
railties/lib/rails/source_annotation_extractor.rb Outdated
pattern = value.call(tag)
break
end
end

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Mar 14, 2014

Member

You could use detect here:

extension = self.class.extensions.detect do |regexp, _block|
  regexp.match(item)
end

if extension
  pattern = extension.last
  results.update(extract_annotations_from(item, pattern))
end
@senny
senny reviewed Mar 14, 2014
View changes
railties/lib/rails/source_annotation_extractor.rb Outdated
/\.(css|scss|sass|less|js)$/ => ->(tag) { /\/\/\s*(#{tag}):?\s*(.*)$/ },
/\.erb$/ => ->(tag) { /<%\s*#\s*(#{tag}):?\s*(.*?)\s*%>/ },
/\.haml$/ => ->(tag) { /-\s*#\s*(#{tag}):?\s*(.*)$/ },
/\.slim$/ => ->(tag) { /\/\s*\s*(#{tag}):?\s*(.*)$/ } }

This comment has been minimized.

Copy link
@senny

senny Mar 14, 2014

Member

now that we have an API extensions like scss, sass, less, haml, slim, etc. should be registered in the corresponding gems. The defaults should only include extensions that rails supports out of the box.

@senny
Copy link
Member

senny commented Mar 14, 2014

we will need test cases.

@robertomiranda
Copy link
Contributor Author

robertomiranda commented Mar 14, 2014

done, i guess 😄

@senny
senny reviewed Mar 14, 2014
View changes
railties/lib/rails/source_annotation_extractor.rb Outdated

register_extensions("builder", "rb", "rake") { |tag| /#\s*(#{tag}):?\s*(.*)$/ }
register_extensions("css", "js") { |tag| /\/\/\s*(#{tag}):?\s*(.*)$/ }
register_extensions("erb") { |tag| /<%\s*#\s*(#{tag}):?\s*(.*?)\s*%>/ }

This comment has been minimized.

Copy link
@senny

senny Mar 14, 2014

Member

are these all extensions supported by default? What about yml for example?

This comment has been minimized.

Copy link
@robertomiranda

robertomiranda Mar 14, 2014

Author Contributor

Right, I removed all the extensions that can be added in the corresponding gems. I'm going to add the support for yml 👍

This comment has been minimized.

Copy link
@robertomiranda

robertomiranda Mar 14, 2014

Author Contributor

Also I missed up .ruby extension

… we have an API for register it in the corresponding gems
@robin850 robin850 added this to the 4.2.0 milestone Mar 15, 2014
@robin850 robin850 added the railties label Mar 15, 2014
@robin850
Copy link
Member

robin850 commented Mar 15, 2014

@robertomiranda : Thanks for your contribution! It looks like your tests are failing though. Would you mind having a look ? Could you also add a changelog entry please ?

Also I don't know what do you guys think but I also thought about adding such feature for a while but at the railtie level. What do you think about providing a wrapper around SourceAnnotationExtractor::Annotation.register_extensions in railties' configuration like:

config.annotations.register_extensions("coffee") do
  /regex/
end
app_file "app/assets/javascripts/application.js", "// TODO: note in js"
app_file "app/assets/stylesheets/application.css", "// TODO: note in css"
app_file "app/assets/stylesheets/application.css.scss", "// TODO: note in scss"

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Mar 16, 2014

Member

We should keep scss and sass

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Mar 16, 2014

Member

Nevermind, this will be sass-rails job.

@rafaelfranca
Copy link
Member

rafaelfranca commented Mar 16, 2014

👍 for @robin850 idea.

@robertomiranda
Copy link
Contributor Author

robertomiranda commented Mar 16, 2014

@robin850 👍 for the wrapper. I'm going to review the tests

@robertomiranda
Copy link
Contributor Author

robertomiranda commented Mar 16, 2014

  • Tests Fixed
  • Changelog entry added
  • Config wrapper Done
@senny
senny reviewed Mar 17, 2014
View changes
railties/CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
* Add Public Api for Register New Extensions for Rake Notes.

This comment has been minimized.

Copy link
@senny

senny Mar 17, 2014

Member

can we update it to:

Add public API to register new extensions for rake notes.

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Mar 17, 2014

Member

Would be good to add an example on how to register new stuff, and maybe a guides update if we have something about rake notes in there.

@senny
senny reviewed Mar 17, 2014
View changes
railties/test/application/rake/notes_test.rb Outdated
SourceAnnotationExtractor::Annotation.register_extensions("test1", "test2"){ |tag| /#{tag}/ }
assert_not_nil SourceAnnotationExtractor::Annotation.extensions[/\.(test1|test2)$/]
assert_nil SourceAnnotationExtractor::Annotation.extensions[/\.(haml)$/]
end

This comment has been minimized.

Copy link
@senny

senny Mar 17, 2014

Member

we should also verify that the registered extension is picked up from rake notes

@robertomiranda
Copy link
Contributor Author

robertomiranda commented Mar 17, 2014

All done

guilleiguaran added a commit that referenced this pull request Mar 17, 2014
Add Public Api for Register New Extensions for Rake Notes
@guilleiguaran guilleiguaran merged commit 4a69c93 into rails:master Mar 17, 2014
1 check was pending
1 check was pending
default The Travis CI build is in progress
Details
@guilleiguaran
Copy link
Member

guilleiguaran commented Mar 17, 2014

Thanks!!!

@robertomiranda robertomiranda deleted the robertomiranda:rake-notes.config branch Mar 17, 2014
@guilleiguaran
Copy link
Member

guilleiguaran commented Mar 17, 2014

oops, next time please squash everything in a single commit, please 😁

@jeremy
Copy link
Member

jeremy commented Mar 17, 2014

+squash please.

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

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.