Add Support for Ignoring Models with config.ignored_models#16
Add Support for Ignoring Models with config.ignored_models#16westonganger wants to merge 2 commits into7even:masterfrom
config.ignored_models#16Conversation
|
Sorry I didn't mention this earlier but I don't think we need another means of configuration since the gem already has one. Could you please just add the |
|
Jeez that sucks. I guess I didn't read the readme well enough because there is no configurator example shown. |
westonganger
force-pushed
the
issue_13
branch
from
13fe838 to
72c38e7
Compare
westonganger
changed the title
config.ignored_models
westonganger
force-pushed
the
issue_13
branch
from
11ff57e to
86a26ed
Compare
|
I've added specs however I cannot test ignored_models during annotation as the test suite doesn't seem to support the implemented method for translating the file name to class. Likely you would need to add a full Rails app to the test suite. |
7even
left a comment
7even
left a comment
There was a problem hiding this comment.
It seems like you are testing in the wrong place. You changed the Annotate.models method but you are testing it in the specs for Annotate::File which doesn't call it.
Actually it's the reverse - Annotate.annotate uses Annotate.models to get a list of models to annotate and then calls Annotate::File#annotate_with on each of them.
I would suggest adding a test for Annotate.models to check that it doesn't include the ignored models.
|
|
||
| class User < ActiveRecord::Base | ||
| ### Define User constant | ||
| end |
| # config.yard = false | ||
| # | ||
| # # Define any models to be skipped by Annotate | ||
| # config.ignored_models = [SomeIgnoredModel, AnotherIgnoredModel] |
There was a problem hiding this comment.
I see a potential problem here: AFAIK rails runs initializers before loading the application code so referring to the model classes as constants here will either fail or autoload them, possibly before other initializers are fired (and this doesn't sound good to me).
What if instead of class constants themselves we will use their names as strings? Like %w[SomeIgnoredModel AnotherIgnoredModel]. What do you think?
|
|
||
| private | ||
|
|
There was a problem hiding this comment.
I prefer to add empty lines only before public/protected/private keywords, not after them; please don't change it.
| def configurator | ||
| @configurator ||= Configurator.new | ||
| end | ||
|
|
|
|
||
| private | ||
|
|
| @yard = false | ||
| @ignored_models = [] | ||
| end | ||
|
|
|
|
||
| class User < ActiveRecord::Base | ||
| has_many :posts | ||
| end |
There was a problem hiding this comment.
This is not the expected result here as the ignored file will not be touched at all - the gem won't erase the old annotation. This is what should be expected.
There was a problem hiding this comment.
But since this is a wrong place to test ignored_models you can just delete this.
|
I have some other more important tasks on the go right now. Do you have any interest in picking up this PR and finishing the specs for it? |
|
@westonganger currently I can't, sorry. Maybe in a few weeks. |
2 participants
Solves #13