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

Ensure reset of deferred validation #481

Merged
merged 3 commits into from Dec 15, 2022

Conversation

dpep
Copy link
Contributor

@dpep dpep commented Oct 29, 2022

I was digging through this code and noticed a few things that could be tightened up (it's generally an awesome library though, so thanks!). Forgive me if this is unwelcomed feedback.

There is an edge case where the block in defer_validation could raises an exception and validation_deferred isn't reset properly. Perhaps contrived, but I thought it worth addressing. The rest are more cosmetic suggestions to make this code simpler / more Rubyish

@dpep dpep force-pushed the ensure-validation_deferred branch from 4d66c27 to a1da0c1 Compare October 29, 2022 22:23
lib/addressable/uri.rb Outdated Show resolved Hide resolved
@dentarg
Copy link
Collaborator

dentarg commented Oct 30, 2022

The rest are more cosmetic suggestions to make this code simpler / more Rubyish

Can you put those in a separate PR?

@dpep dpep force-pushed the ensure-validation_deferred branch from a1da0c1 to b65c1c3 Compare October 30, 2022 16:13
@dpep dpep mentioned this pull request Oct 30, 2022
@dpep
Copy link
Contributor Author

dpep commented Oct 30, 2022

@dentarg : spun out into #482 per your request

@dentarg dentarg changed the title ensure validation_deferred Ensure reset of deferred validation Dec 15, 2022
@dentarg dentarg merged commit 32bd534 into sporkmonger:main Dec 15, 2022
@dpep dpep deleted the ensure-validation_deferred branch December 15, 2022 20:29
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.

None yet

3 participants