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

Delete documentation inconsistency 'finally' for AR callbacks [ci skip] #35303

Merged
merged 4 commits into from Feb 19, 2019

Conversation

soartec-lab
Copy link
Contributor

The preamble 'finally' is described in item 8.2 of the AR callback.
However, since it is not the last item, the expression 'finally' is contradictory.

When item 8.3 was added, I guessed that item 8.2 was supposed to be "finally" deleted, so I deleted it.

Since it is not the last item, then 'finally' is inconsistent
@rails-bot rails-bot bot added the docs label Feb 17, 2019
@kamipo
Copy link
Member

kamipo commented Feb 17, 2019

Looks like we have one another "Finally" in the guides.

Finally, it's possible to associate `:if` and `:unless` with a `Proc` object

@soartec-lab
Copy link
Contributor Author

Is this also inconsistent with 'active_record validations.md'?
Or should we interpret the preamble 'finally' as 'fainally' in the description of ': if,: unless'?

@soartec-lab
Copy link
Contributor Author

I also deleted 'finally' in 'active_record_validations.md' @kamipo taught.

guides/source/active_record_callbacks.md Outdated Show resolved Hide resolved
guides/source/active_record_validations.md Outdated Show resolved Hide resolved
kamipo and others added 2 commits February 19, 2019 09:46
Co-Authored-By: soartec-lab <info@soartec-lab.work>
Co-Authored-By: soartec-lab <info@soartec-lab.work>
@soartec-lab
Copy link
Contributor Author

@kamipo

Thank you for review.
We have committed the proposal.

@kamipo
Copy link
Member

kamipo commented Feb 19, 2019

I digged the history, but the items are not the last item from the first merge from docrails.
Anyway, the "Finally" doesn't make sense.

@kamipo kamipo merged commit 02e6abd into rails:master Feb 19, 2019
@soartec-lab
Copy link
Contributor Author

merge Thank you.
I gained good insight through this contribution.

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.

None yet

2 participants