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

Only set the attribute value in reify when it's different #1473

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

Conversation

tubaxenor
Copy link

  • Wrote good commit messages.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new
    code introduces user-observable changes.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

This is related to https://github.com/paper-trail-gem/paper_trail/pull/1468/files#r1601766553

We shouldn't set something again if the value is no difference from the original value. In some cases such like read only attributes might already set with a default value and will not change over time, this is helpful and will not violate Rails 7.1's new default config raise_on_assign_to_attr_readonly.

@jaredbeck
Copy link
Member

Looks good, please add an entry to the Changlog under "Fixed"

@jaredbeck
Copy link
Member

@ngan @professor @technicalpickles Thoughts?

@ngan
Copy link

ngan commented May 28, 2024

Yeah, I think this is a reasonable approach. @tubaxenor can make the request changeling updates when he's back online.

@tubaxenor tubaxenor requested a review from jaredbeck May 29, 2024 00:39
@fynsta
Copy link
Contributor

fynsta commented Aug 20, 2024

We also ran into this problem when upgrading the Rails version. What's the status on this PR?

@ngan
Copy link

ngan commented Nov 14, 2024

I wanted to x-post this thread:
paper-trail-gem/paper_trail#1468 (files)

I'm not fully convinced that this is the right approach to solving this problem. See thread for details.

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.

4 participants