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

[ci skip] add class level documentation to ActiveModel::Type::Boolean #25575

Conversation

taboularasa
Copy link

Summary

adds documentation of the behaviors of type coercion at the class level to ActiveModel::Type::Boolean

Other Information

@sgrif per our conversation last week, let me know if this is what you had in mind.

@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 @eileencodes (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.

@maclover7
Copy link
Contributor

r? @sgrif

# Values set from user or database input will first be coerced into a
# appropriate ruby type.
#
# - Values specified in +FALSE_VALUES+ will be coerced to +false+
Copy link
Contributor

Choose a reason for hiding this comment

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

We should mention the most common ones explicitly, false, "0", 0, and 'f'.

@taboularasa taboularasa force-pushed the taboularasa/update-docs-ActiveModel__Type__Boolean branch from 8b043c1 to 11cc30c Compare June 29, 2016 21:22
@taboularasa
Copy link
Author

@sgrif @rafaelfranca all feedback addressed

# appropriate ruby type.
#
# - Values specified in +FALSE_VALUES+ will be coerced to +false+.
# - e.g. "false", "f" , "0", and +0+ will all be coerced to +false+
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer this to be a bit more human. Something like:

false, "f", "0", 0, or any other value in FALSE_VALUES will be coerced to false

@sgrif
Copy link
Contributor

sgrif commented Jun 29, 2016

TIL I have more opinions than I realized about how to document type coercion to booleans.

@sgrif
Copy link
Contributor

sgrif commented Jun 29, 2016

Thanks for working on this @taboularasa ❤️

@taboularasa
Copy link
Author

@sgrif thanks for your feedback and please keep it coming, I would like to get a good sense for this before I work on documenting the rest of the ActiveModel::Type namespace

@taboularasa taboularasa force-pushed the taboularasa/update-docs-ActiveModel__Type__Boolean branch from 11cc30c to 5702045 Compare June 30, 2016 00:57
@taboularasa
Copy link
Author

@sgrif ok all feedback addressed again, please take another look when you get a chance

@sgrif
Copy link
Contributor

sgrif commented Jun 30, 2016

I'm about to head to bed, and I'm in Europe for a few weeks as of tomorrow, but I will try to take a look soon (but it won't be until Saturday at the earliest)

@taboularasa
Copy link
Author

Np, take your time

#
# === Coercion
# Values set from user input will first be coerced into the appropriate ruby type.
# Coercion behaivor is roughly mapped to Ruby's boolean semantics.
Copy link
Member

Choose a reason for hiding this comment

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

s/behaivor/behavior

@sgrif
Copy link
Contributor

sgrif commented Jul 11, 2016

LGTM other than the typo.

add documentation of the behaviors of type coercion at the class level
@taboularasa taboularasa force-pushed the taboularasa/update-docs-ActiveModel__Type__Boolean branch from 5702045 to 3691c75 Compare July 11, 2016 14:30
@taboularasa
Copy link
Author

typo fixed and rebased

@sgrif sgrif merged commit 968b59a into rails:master Jul 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants