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 Support for Ignoring Models with `config.ignored_models` #16

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@westonganger
Copy link
Contributor

commented Feb 7, 2019

Solves #13

@7even

This comment has been minimized.

Copy link
Owner

commented Feb 7, 2019

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 ignored_models option there?

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

Jeez that sucks. I guess I didn't read the readme well enough because there is no configurator example shown.

@westonganger westonganger force-pushed the westonganger:issue_13 branch from 13fe838 to 72c38e7 Feb 7, 2019

@westonganger westonganger changed the title Add Support for Ignoring Models using .annotate_ignore Add Support for Ignoring Models with `config.ignored_models` Feb 7, 2019

@westonganger westonganger force-pushed the westonganger:issue_13 branch from 11ff57e to 86a26ed Feb 10, 2019

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

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
Copy link
Owner

left a 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

This comment has been minimized.

Copy link
@7even

7even Feb 10, 2019

Owner

Is this being used anywhere?

# config.yard = false
#
# # Define any models to be skipped by Annotate
# config.ignored_models = [SomeIgnoredModel, AnotherIgnoredModel]

This comment has been minimized.

Copy link
@7even

7even Feb 10, 2019

Owner

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

This comment has been minimized.

Copy link
@7even

7even Feb 10, 2019

Owner

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

This comment has been minimized.

Copy link
@7even

7even Feb 10, 2019

Owner

This empy line is redundant.

private

This comment has been minimized.

Copy link
@7even

7even Feb 10, 2019

Owner

And here.

end

This comment has been minimized.

Copy link
@7even

7even Feb 10, 2019

Owner

And here.

# enCoding: utf-8
# frozen_string_literal: true
# warn_indent: true
class User < ActiveRecord::Base
has_many :posts
end

This comment has been minimized.

Copy link
@7even

7even Feb 10, 2019

Owner

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.

This comment has been minimized.

Copy link
@7even

7even Feb 10, 2019

Owner

But since this is a wrong place to test ignored_models you can just delete this.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

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?

@7even

This comment has been minimized.

Copy link
Owner

commented Feb 12, 2019

@westonganger currently I can't, sorry. Maybe in a few weeks.

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.