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

Added SanitizeHelper to rails guide docs [ci skip] #11617

Merged
merged 1 commit into from Aug 14, 2013

Conversation

swooop
Copy link
Contributor

@swooop swooop commented Jul 26, 2013

No description provided.

@@ -1520,6 +1520,59 @@ number_with_precision(111.2345) # => 111.235
number_with_precision(111.2345, 2) # => 111.23
```

This is for a single use.

Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand this line. Could you explain what you mean please? :-)

@robin850
Copy link
Member

Hello,

Thanks for your contribution! ❤️ I've added some comments. Could you just wrap these helpers with a title (here SanitizeHelper) please?

@swooop
Copy link
Contributor Author

swooop commented Jul 26, 2013

Super pwned!

Thanks for the feedback I'll get right on it.
-- 
Toby Sims
Sent with Airmail

On 26 July 2013 at 17:39:26, Robin Dupret (notifications@github.com) wrote:

Hello,

Thanks for your contribution! I've added some comments. Could you just wrap these helpers with a title (here SanitizeHelper) please?


Reply to this email directly or view it on GitHub.

@swooop
Copy link
Contributor Author

swooop commented Jul 26, 2013

Oops. My bad, too many files open.

@robin850
Copy link
Member

Could you just squash your commits into a single one please?

$ git rebase -i HEAD~3
# Replace pick with squash except for the first commit
$ git push origin +doc_sanitize

@swooop
Copy link
Contributor Author

swooop commented Jul 26, 2013

You learn something new every day…

-- 
Toby Sims
Sent with Airmail

On 26 July 2013 at 17:56:16, Robin Dupret (notifications@github.com) wrote:

Could you just squash your commits into a single one please?

$ git rebase -i HEAD~3

Replace pick with squash except for the first commit

$ git push origin +doc_sanitize

Reply to this email directly or view it on GitHub.

@robin850
Copy link
Member

You learn something new every day…

Yeah, especially with Git!

Didn't see any error with the snippet I've posted? Seems like this hasn't applied correctly.

Added SanitizeHelper to rails guide docs [ci skip]

Added SanitizeHelper to rails guide docs update [ci skip]
@swooop
Copy link
Contributor Author

swooop commented Jul 29, 2013

I think I won. Curse my poor spelling.

@robin850
Copy link
Member

Awesome, thanks you! :-)

LGTM /cc @fxn

@swooop
Copy link
Contributor Author

swooop commented Jul 29, 2013

No problem, thanks for being so helpful!
On Jul 29, 2013 10:00 AM, "Robin Dupret" notifications@github.com wrote:

Awesome, thanks you! :-)

LGTM /cc @fxn https://github.com/fxn


Reply to this email directly or view it on GitHubhttps://github.com//pull/11617#issuecomment-21707071
.

@senny
Copy link
Member

senny commented Aug 14, 2013

I feel like this kind of documentation is too similar to the Rails API documentation I don't see a good reason to duplicate the documentation on the method level. I know that there are already modules documented that way in this guide, I just don't think it's particularly useful.

@steveklabnik @fxn what do you guys think?

@steveklabnik
Copy link
Member

Yeah it's a middle ground. We have some guides that are like this; the validations guide, for example. I am also a vague 👎 , but since we have other things like this, maybe this should be in too.

@fxn?

@fxn
Copy link
Member

fxn commented Aug 14, 2013

Yeah, there is no good solution I am afraid.

Some guides are more detailed, the most extreme one is the Active Support guide that not only documents at the API level, but it is more informative than the API itself for some methods (motivation is to provide a central point to read about extensions to core classes).

This guide has also a section where helpers are listed... gray area.

In general I believe we should avoid duplication. Find a balanced point and at most cross-link ("read the API documentation for further details", something like that). Which reminds me that I need to figure out a way to cross-link.

The patch is fine for me though, no reason strong enough not to apply.

fxn added a commit that referenced this pull request Aug 14, 2013
Added SanitizeHelper to rails guide docs [ci skip]
@fxn fxn merged commit aa6c651 into rails:master Aug 14, 2013
@robin850
Copy link
Member

@fxn @steveklabnik @senny : Maybe we could just put a list of links pointing to the different methods/helpers and try to enhance as possible the API to avoid duplication? If you want me to help you at this level, I would be glad!

@fxn
Copy link
Member

fxn commented Aug 15, 2013

@robin850 I think we could leave these as they are by now.

@swooop swooop deleted the doc_sanitize branch September 6, 2013 15:17
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.

None yet

5 participants