Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Check Encodings before calling force_encoding in Addressable::URI #341
Check Encodings before calling force_encoding in Addressable::URI #341
Changes from 6 commits
2e08cef
52c524b
8194318
02c6912
ca5a303
781ee9a
b5e929b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to have this test not care at all which error type is raised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I'm following... it says
should not allow destructive operations
and we expect@uri.normalize!
to raise RuntimeError/FrozenError (can't modify frozen String
). Is that because you shouldn't be able to use#normalize!
on a frozenAddressable::URI
? Maybe we can make that even more clear in the test? (The@uri.freeze
happens way up in thebefore
block so it is easy to miss)Maybe
Addressable::URI#normalize!
should detect that the URI is frozen and raise an error defined by Addressable? (FrozenURIError
perhaps?)Ah, this particular spec doesn't test the changes made in this branch, I ran this against Addressable 2.8.0
Perhaps we shouldn't change this behaviour as I suggested above
@sporkmonger should we just check error message? Expect
can't modify frozen Addressable::URI
. Those can change from Ruby to Ruby version too, but probably unlikely in this case IMHO.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly I'd prefer to have the test make the same checks regardless of Ruby version. If the error message is consistent between versions but the type is not, it seems fine to check the error message, but if that's inconsistent too, I think it's fine to verify that an error, any error, has been thrown.
I think I'd rather not do a custom error type because I think if we did a custom type it should probably inherit from
FrozenError
on versions of Ruby that have it, and that just shifts the problem out of the tests and into the library itself. I'm not sure that's an improvement.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have the test make the same checks regardless of Ruby version. If we look at the hierarchy of exceptions at https://ruby-doc.org/core-2.6.5/Exception.html we can see that FrozenError is a subclass of RuntimeError. That should never change really. We can use
#is_a?
and always check if it is a RuntimeError:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(We don't need to check the message)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other changes required? I've implemented the above changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now, thanks!