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

Skip ApplicationRecord #10

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@westonganger
Copy link
Contributor

commented Jan 22, 2019

Depending on the resolution to #11 this may not be needed

@@ -45,6 +45,7 @@ def models

klass = class_name_for(short_path)
next unless klass < ActiveRecord::Base # collect only AR::Base descendants
next if klass.name == "ApplicationRecord" # skip ApplicationRecord

This comment has been minimized.

Copy link
@Morozzzko

Morozzzko Jan 25, 2019

Contributor

will this work if my ApplicationRecord is namespaced? Like MyFancy::Domain::ApplicationRecord?

This comment has been minimized.

Copy link
@westonganger

westonganger Jan 25, 2019

Author Contributor

Not sure but im sure this is a silly PR as it has such a limited reach.

We need some other way to ignore certain files. Maybe we could have an .annotate_ignore file in the project root where we list these?

This comment has been minimized.

Copy link
@Morozzzko

Morozzzko Jan 26, 2019

Contributor

I believe that we don't need much configuration, but it would be nice to ignore conventional stuff like ApplicationRecord

Also,

irb(main):019:0> Foo::Bar::Quux.name
=> "Foo::Bar::Quux"

So we could replace it with:

irb(main):020:0> *, name = "Foo::Bar::Quux".split(':')
=> ["Foo", "", "Bar", "", "Quux"]
irb(main):023:0> *, name = "Quux".split(':')
=> ["Quux"]

This comment has been minimized.

Copy link
@westonganger

westonganger Jan 26, 2019

Author Contributor

Yeah but what happens if someone names an actual model application record then we have an issue. I would prefer something like described in #13

This comment has been minimized.

Copy link
@Morozzzko

Morozzzko Jan 26, 2019

Contributor

I wonder how many people actually see this problem

I can tell that my project only has two application records at this moment. Both are called ApplicationRecord

A simple fix should suffice for a while. Better cover 95% of use cases while we think about custom configuration

This comment has been minimized.

Copy link
@Morozzzko

Morozzzko Jan 26, 2019

Contributor

Sounds like a plan

This comment has been minimized.

Copy link
@7even

7even Jan 27, 2019

Owner

I fixed that in 560af45. @westonganger @Morozzzko please try gem 'active_record-annotate', github: '7even/active_record-annotate' in your apps to confirm it's working.

This comment has been minimized.

Copy link
@Morozzzko

Morozzzko Jan 27, 2019

Contributor

Checked:

  • If there are some annotations, db:annotte won't remove them
  • If I remove them manually and re-run db:annotate, the gem won't add them
  • If I rollback to the previous version, db:annotate would override my changes and add the error

Works well now

This comment has been minimized.

Copy link
@7even

7even Jan 27, 2019

Owner
  • If there are some annotations, db:annotte won't remove them

Yep, that's expected behavior - it only touches the files it is going to annotate, and now this process completely ignores abstract classes. You'll have to manually remove old buggy annotations.

This comment has been minimized.

Copy link
@Morozzzko

Morozzzko Jan 27, 2019

Contributor

Yeah, that's alright. Pretty much expected it.

Thanks!

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.