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

Avoid chaining duplicated semian identifiers in error messages #423

Merged
merged 1 commit into from
Oct 7, 2022

Conversation

casperisfine
Copy link
Contributor

When errors are created from other errors messages they might already have a semian identifier in their message

lib/semian.rb Show resolved Hide resolved
test/adapter_test.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@adrianna-chang-shopify adrianna-chang-shopify left a comment

Choose a reason for hiding this comment

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

Makes sense to me! Is there anywhere in the repo where we're currently constructing an error from another error's message where this was a problem? Or is this just a future-proofing fix?

lib/semian.rb Outdated
else
super
prefix = "[#{@semian_identifier}] "
# When errors are created from other errors messages they might
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# When errors are created from other errors messages they might
# When errors are created from other errors' messages they might

Or maybe singularize it?

Suggested change
# When errors are created from other errors messages they might
# When an error is created from another error's message it might
# already have a semian identifier in the message

@casperisfine
Copy link
Contributor Author

Is there anywhere in the repo where we're currently constructing an error from another error's message where this was a problem?

Not in the repo, but redis-rb 5.0 does this a lot with redis-client errors: https://github.com/redis/redis-rb/blob/d4b24e1e4fac7b9e3fee01947bf400f52ea500cf/lib/redis/client.rb#L118-L126

@casperisfine casperisfine force-pushed the avoid-nested-identifiers branch 2 times, most recently from 086b048 to e568d32 Compare October 7, 2022 16:08
@miry miry added the Semian label Oct 7, 2022
@miry
Copy link
Contributor

miry commented Oct 7, 2022

I think it should be mentioned in Changelog as well.

When errors are created from other errors messages they might
already have a semian identifier in their message
@casperisfine
Copy link
Contributor Author

Done.

@casperisfine casperisfine merged commit 5f6f526 into master Oct 7, 2022
@casperisfine casperisfine deleted the avoid-nested-identifiers branch October 7, 2022 16:14
@miry miry mentioned this pull request Dec 13, 2022
9 tasks
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

4 participants