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 warning about deduplication of after_xxx_commit shortcuts #51184

Merged

Conversation

ConfusedVorlon
Copy link
Contributor

Motivation / Background

I'm not the first person to be bitten by deduplication of after_xxx_commit shortcuts.

#40648
#51171

Detail

Adds documentation to warn about this.

@vipulnsward vipulnsward merged commit 2abee30 into rails:main Feb 25, 2024
3 checks passed
@vipulnsward
Copy link
Member

Thanks!

zzak added a commit to zzak/rails that referenced this pull request May 23, 2024
…fter_commit_deduplication"

This reverts commit 2abee30, reversing
changes made to e34a0ee.

A warning on each method is excessive and the warning should already be covered under the guide:
https://edgeguides.rubyonrails.org/active_record_callbacks.html#transaction-callbacks
zzak added a commit to zzak/rails that referenced this pull request May 23, 2024
…on/document_after_commit_deduplication"

This reverts commit 2abee30, reversing
changes made to e34a0ee.

A warning on each method is excessive and the warning should already be covered under the guide:
https://edgeguides.rubyonrails.org/active_record_callbacks.html#transaction-callbacks
@zzak
Copy link
Member

zzak commented May 23, 2024

This is already documented here:
https://edgeguides.rubyonrails.org/active_record_callbacks.html#transaction-callbacks

Using both after_create_commit and after_update_commit with the same method name will only allow the last callback defined to take effect, as they both internally alias to after_commit which overrides previously defined callbacks with the same method name.

@ConfusedVorlon
Copy link
Contributor Author

@zzak So it is - however people don't necessarily check/remember that guide.

Indeed, this bit me because I was reading the method's docs, rather than thinking to check the overview guide for this.

Personally, I don't see any problem with duplicating a warning so that people can get the info at the method definition as well as if they RTM.

zzak added a commit that referenced this pull request May 23, 2024
Revert "Merge pull request #51184 from ConfusedVorlon/document_after_commit_deduplication"
zzak added a commit that referenced this pull request May 23, 2024
[7-2-stable] Revert "Merge pull request #51184 from ConfusedVorlon/document_after_commit_deduplication"
@zzak
Copy link
Member

zzak commented May 23, 2024

@ConfusedVorlon Thanks for your PR, and apologies for not getting feedback sooner.

I would prefer something in the form as I mentioned here: #51886 (comment)

@ConfusedVorlon
Copy link
Contributor Author

I gotta say - this does not fill me with enthusiasm for trying to improve the docs.
I saw a problem, I offered a fix, it was merged.

Now you think that the red warning in the guide is sufficient - and you remove the warnings at the method definition.

If you think that's better - so be it.
To my mind - the big red warning in the guide only emphasises the need for warnings at the method definition.

Personally, I would suggest that if you want to improve the style of documentation after changes are committed, then a pull request would be a better solution than a revert.

@zzak
Copy link
Member

zzak commented May 23, 2024

I disagree that there needs to be a warning at every method definition, but open to adding a note in the top-level ClassMethods overview.

This is consistent with the rest of our documentation, please show me if that is not the case.

As for the method naming, for example we use after_*_commit in the guides where we typically use wildcard notation for dynamic methods (See: Dirty).

It's on me because if I could have reviewed your PR sooner, I would have given you that guidance sooner and hopefully we could gain some momentum together.

If you want to submit the other PR feel free to ping me.

@ConfusedVorlon
Copy link
Contributor Author

ConfusedVorlon commented May 23, 2024

If you want to submit the other PR feel free to ping me.

Nope. I'm done.
I'll remember this gotcha. The next person will have to find out the hard way.

btw - this wasn't submitted out of nowhere. I did ask for advice on format first.
#51171

zzak added a commit to zzak/rails that referenced this pull request May 31, 2024
zzak added a commit to zzak/rails that referenced this pull request May 31, 2024
zzak added a commit that referenced this pull request May 31, 2024
…tion

/cc #51184, #51171, #40648, #48910

Co-authored-by: Rob Jonson <rob@hobbyistsoftware.com>
xjunior pushed a commit to xjunior/rails that referenced this pull request Jun 9, 2024
…fter_commit_deduplication"

This reverts commit 2abee30, reversing
changes made to e34a0ee.

A warning on each method is excessive and the warning should already be covered under the guide:
https://edgeguides.rubyonrails.org/active_record_callbacks.html#transaction-callbacks
xjunior pushed a commit to xjunior/rails that referenced this pull request Jun 9, 2024
jianbo pushed a commit to jianbo/fix-association-initialize-order that referenced this pull request Jul 8, 2024
…fter_commit_deduplication"

This reverts commit 2abee30, reversing
changes made to e34a0ee.

A warning on each method is excessive and the warning should already be covered under the guide:
https://edgeguides.rubyonrails.org/active_record_callbacks.html#transaction-callbacks
jianbo pushed a commit to jianbo/fix-association-initialize-order that referenced this pull request Jul 8, 2024
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.

None yet

3 participants