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.

Sign up for GitHub

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

Jump to bottom

Requesting documentation update #22240

Merged
merged 1 commit into from
May 29, 2016
Merged

Requesting documentation update #22240

merged 1 commit into from
May 29, 2016

Conversation

resource11
Copy link
Contributor

Hi there!

I made a suggested edit to the section that gives a 'word of warning' with respect to creating associations with the same name as ActiveRecord::Base instance methods. It'd be great to have a hyperlink to the list of ActiveRecord::Base instance methods so we can more easily know which association names we should avoid to minimize conflicts.

** I originally made this pull request on an incorrect branch, so I'm resubmitting now, comparing against the master branch.

Also, it was requested in the last pull request that I don't link to APIdock--since it is not the official documentation site-- and to use Rdoc to link internally. I'd be happy to link internally if I could find the actual location of where one can find the list of ActiveRecord::Base instance methods elsewhere within the documentation site. Can someone point me to that? It really would help provide clarification for the new-to-RoR crowd...

Thanks!

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @matthewd (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@arthurnn
Copy link
Member

arthurnn commented Dec 5, 2015

thanks for the PR @resource11 . I like the text change, and indeed think we should change the link. Our official docs are on http://api.rubyonrails.org/ . The AR::Base class docs are in http://api.rubyonrails.org/classes/ActiveRecord/Base.html , However as far as I can tell, there is no list of methods there. So I am not sure where else we can find that list.
@rafaelfranca thoughts?

maclover7
maclover7 reviewed Jan 10, 2016
View reviewed changes
activerecord/lib/active_record/associations.rb
# ActiveRecord::Base. Since the association adds a method with that name to
# its model, it will override the inherited method and break things.
# For instance, +attributes+ and +connection+ would be bad choices for association names.
# Don't create associations that have the same name as [instance methods](http://apidock.com/rails/ActiveRecord/Base) of
Copy link
Contributor

Choose a reason for hiding this comment

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

The official Rails code page for AR::Base would be: http://api.rubyonrails.org/classes/ActiveRecord/Base.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dug into that link and found this:

http://api.rubyonrails.org/classes/ActiveRecord/Core.html#method-c-new, which pops the user to a list of public instance methods. Is this link sufficient?

@maclover7
Copy link
Contributor

Thanks for updating! Just so you know for the future, you can put [ci skip] into your commit message to avoid running the tests on Travis.

@maclover7
Copy link
Contributor

Hmm, still not sure that's the right link to send people to. @rafaelfranca, thoughts?

r? @rafaelfranca

@rails-bot rails-bot assigned rafaelfranca and unassigned matthewd Jan 10, 2016
@resource11
Copy link
Contributor Author

Thanks for the tip on the commit message. Looking forward to clarification on the correct link.

@arthurnn
Copy link
Member

The issue that I see about this, is that not all public methods are on the Base docs. as we have a lot of modules we include on Base. =/
I am not sure what the best solution is. I guess we could just say in the docs to no override methods like +connection+, etc, but dont point any link.

@resource11
Copy link
Contributor Author

Hmmm, could there at least be a list of 3-4 methods as an example to give more guidance with respect to names to avoid? Provide a breadcrumb trail to start the hunt for more info?

@arthurnn
Copy link
Member

I think a few examples, are a good idea. Like we have right now: For instance, +attributes+ and +connection+ ...
One way to figure out which methods are public there is: ActiveRecord::Base.public_methods, Which is a list of 600+ methods at the moment.

@resource11
Copy link
Contributor Author

^^ That information is a great thread for beginner users to pull! Can that be added to the documentation?

arthurnn
arthurnn reviewed Mar 8, 2016
View reviewed changes
activerecord/lib/active_record/associations.rb
# ActiveRecord::Base. Since the association adds a method with that name to
# its model, it will override the inherited method and break things.
# For instance, +attributes+ and +connection+ would be bad choices for association names.
# Don't create associations that have the same name as [instance methods](http://api.rubyonrails.org/classes/ActiveRecord/Core.html#method-c-new) of
Copy link
Member

Choose a reason for hiding this comment

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

the link should be http://api.rubyonrails.org/classes/ActiveRecord/Core.html .

Also, @rafaelfranca is it OK to add markdown style links to these docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification on the correct link, @arthurnn.

Considering the link is now listed on line 303, should lines 308 and 309 be removed?
== Auto-generated methods
See also Instance Public methods below for more details.

Copy link
Member

Choose a reason for hiding this comment

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

I dont think we should remove that.
I think this is already a good improvement.
As a final change. can you:

  • update the link, to remove #method-c-new from it.

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Link updated (#method-c-new removed)

@arthurnn
Copy link
Member

Thanks for the update. Can you squash your commits in one and I will merge that. Thanks again

@resource11
Issue 22240: adds link to list of instance methods [ci skip]
79f2f0b 
Update associations.rb

Update associations.rb

updates link to instance methods [ci skip]
@resource11
Copy link
Contributor Author

Commits squashed. Thanks, @arthurnn!

@arthurnn arthurnn merged commit 3f2e83d into rails:master May 29, 2016
@arthurnn
Copy link
Member

thanks, and sorry for letting this get outside my radar.

@resource11
Copy link
Contributor Author

My pleasure! Glad this update got merged into the master branch.

@resource11 resource11 deleted the resource11-patch-1 branch May 30, 2016 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@rafaelfranca rafaelfranca

Labels
activerecord docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@resource11 @rails-bot @arthurnn @maclover7 @matthewd @rafaelfranca