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

Additional checks #82

Closed
wants to merge 1 commit into from
Closed

Additional checks #82

wants to merge 1 commit into from

Conversation

adie
Copy link

@adie adie commented Jun 9, 2012


First check is to support models with no primary key


Also I changed c.is_a?(Class) to Class === c because of the following error:

Unable to annotate my_model.rb: wrong number of arguments(2 for 1)
(.../activesupport-3.2.5/lib/active_support/option_merger.rb:22:in `is_a?')

I didn't find the reason of that error, just tried Class === c and it worked.


And I had to add the last check because of some strange errors from factory_girl:

Unable to annotate my_model.rb: undefined method `include?' for
#<FactoryGirl::Declaration::Implicit:0x9615d74> 
(.../lib/annotate/annotate_models.rb:322:in `block in get_loaded_model')

I've found that it was trying to get ancestors for instance of FactoryGirl::DefinitionProxy, which seems to be making a lot of magic.


@alexch
Copy link
Collaborator

alexch commented Jun 9, 2012

I figured out the same Class === c fix and checked it in yesterday. It's new with 2.3.14, coming from some ActiveSupport method_missing madness.

@adie
Copy link
Author

adie commented Jun 12, 2012

In the migration you can write

create_table :items_users, :id => false do |t|
  t.references :item
  t.references :user
end

When you create habtm associations, you need such a migration, and in some odd cases you may need the model itself for this association.

@@ -82,7 +82,7 @@ def get_schema_info(klass, header, options = {})
attrs = []
attrs << "default(#{quote(col.default)})" unless col.default.nil?
attrs << "not null" unless col.null
attrs << "primary key" if col.name.to_sym == klass.primary_key.to_sym
attrs << "primary key" if klass.primary_key && col.name.to_sym == klass.primary_key.to_sym
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I solve this problem this way:

attrs << "primary key" if col.name.to_sym == klass.primary_key.try(:to_sym)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"try" seems like a fun method. Where is it defined?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know if we can use ActiveSupport methods since some people use
Annotate without Rails.

https://github.com/rails/rails/blob/6ef9fda1a39f45e2d18aba4881f60a19589a2c77/activesupport/lib/active_support/core_ext/object/try.rb#L32

On Fri, Jul 13, 2012 at 4:33 PM, arturtr
[email protected]
wrote:

@@ -82,7 +82,7 @@ def get_schema_info(klass, header, options = {})
attrs = []
attrs << "default(#{quote(col.default)})" unless col.default.nil?
attrs << "not null" unless col.null

  •    attrs << "primary key" if col.name.to_sym == klass.primary_key.to_sym
    
  •    attrs << "primary key" if klass.primary_key && col.name.to_sym == klass.primary_key.to_sym
    

http://api.rubyonrails.org/classes/Object.html#method-i-try


Reply to this email directly or view it on GitHub:
https://github.com/ctran/annotate_models/pull/82/files#r1163133

Alex Chaffee - [email protected]
http://alexchaffee.com
http://twitter.com/alexch

@jmcnevin
Copy link
Contributor

Would really like to see this merged. I can't annotate any Factory Girl factories without it.

@nilbus
Copy link

nilbus commented Jul 19, 2012

@adie Thanks for fixing the is_a? bug - I was running into that too.

@alexch
Copy link
Collaborator

alexch commented Jul 27, 2012

Done. Sadly my git fu was not strong enough to properly credit adie so her fork will appear to be dangling, but the fix is in 076e778

@alexch alexch closed this Jul 27, 2012
lime pushed a commit to lime/annotate_models that referenced this pull request Nov 5, 2013
@ctran ctran added the released label Dec 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants