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

Document sheet_for in README #451

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MicahBrown
Copy link

PR for #447

@tgturner

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 94.155% when pulling 984506d on MicahBrown:document-sheet-for into 6f703f0 on roo-rb:master.

@chopraanmol1
Copy link
Member

@MicahBrown thank you for opening this PR. What about adding a method sheet! (with bang, same functionality as sheet), and add depreciation warning in sheet method which could be removed in some major release. Depreciation warning can also include short description of both sheet_for and sheet! as alternate suggestion

@MicahBrown
Copy link
Author

MicahBrown commented Sep 14, 2018 via email

@chopraanmol1
Copy link
Member

chopraanmol1 commented Sep 14, 2018

default_sheet! and set_default_sheet! will be confusing given than we have default_sheet and default_sheet=

The functionality of sheet and default_sheet= differs which will make it more confusing.

I'm open for better method name over sheet!, but default_sheet! & set_default_sheet! is not a good option.

@MicahBrown
Copy link
Author

MicahBrown commented Sep 14, 2018 via email

@chopraanmol1
Copy link
Member

chopraanmol1 commented Sep 14, 2018

set_default what?

Method name doesn't say it has anything to do with sheet.

And generally in ruby we do not prefer set_ as a prefix (something= is prefered over set_something).

@tgturner
Copy link
Contributor

@MicahBrown I think @chopraanmol1 is getting at a couple of things here:

  1. changing the name from sheet to sheet! implies two things, that it modifies the object it was called on, and will return the object it was called on. Both of those are true in this case so would be nice to communicate to users.

  2. we already have a direct accessor for default_sheet. The implementation between that and sheet are bit nuanced, so probably best to just have both for now.

I would love to see you implement the sheet! change. Let me know if you feel comfortable with that.

@GeoffAbtion
Copy link

GeoffAbtion commented Jan 22, 2020

There is a bunch of discussion in this thread that is not specially relevant to this PR which would be a useful addition to the documentation.

Can we merge this PR and link to this discussion from the appropriate issue thread?

Edit: A possible reason to not merge this: #455 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants