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

ActiveJob: removing redundant if statement on Arguments serialization of String #50127

Conversation

jarenas9539
Copy link

Motivation / Background

After changes from #50090 and #50122, a redundant if statement remains on Arguments#serialize on String related classes serialization. A comment related to this was added here.

Detail

When the begin/rescue statement was added to the else clause the if clause began to be scoped on the rescue clause as StringSerializer is not defined and will throw SerializationError (same current behaviour as other String subclasses without defined serializers). As if clause and rescue clause share same result, the if clause started to be redundant.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

… of String

After changes from rails#50090 and rails#50122, a redundant if statement remains on Arguments#serialize on String related classes serialization. When adding the begin/rescue statement on the else clause the if clause began to be scoped on the rescue clause as StringSerializer is not defined and will throw SerializationError. As if clause and rescue clause share same result, the if clause started to be redundant.
@rails-bot rails-bot bot added the activejob label Nov 21, 2023
@skipkayhil
Copy link
Member

As if clause and rescue clause share same result, the if clause started to be redundant.

While this may be true, I don't think we should make this change. Using rescue for control flow is much worse for performance than a simple if, and since I imagine String is a very common argument to serialize this could have a large impact on applications.

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

2 participants