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

more rubyish #482

Merged
merged 1 commit into from Dec 15, 2022
Merged

more rubyish #482

merged 1 commit into from Dec 15, 2022

Conversation

dpep
Copy link
Contributor

@dpep dpep commented Oct 30, 2022

while working on #481 I noticed a few things that could be tightened up and be made more Rushish. there should be no functionality changes. Forgive me if this is unwelcomed feedback

@@ -851,7 +849,8 @@ def initialize(options={})
self.query_values = options[:query_values] if options[:query_values]
self.fragment = options[:fragment] if options[:fragment]
end
self.to_s

to_s # run path validation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line took me a bit to figure out, so I figured it was comment worthy. based on the specs, it looks like initialize leverages the validation of self.path done in .to_s. but moving it into .validate causes other test failures...

@dentarg
Copy link
Collaborator

dentarg commented Oct 30, 2022

Could give this PR a better title?

@dpep dpep changed the title ensure validation_deferred more rubyish Oct 30, 2022
@dpep
Copy link
Contributor Author

dpep commented Oct 30, 2022

for sure. sorry, that got copied over from the other branch. better? (otherwise, open to suggestions)

end
if port == EMPTY_STR
port = nil
port = nil if port == EMPTY_STR
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this assignment was moved inside if authority != nil where it wasn't before.
Not sure it matters (all tests still pass, but we might lack coverage).

Spotted this when looking at the files instead of the diff.

This PR

if authority != nil
# The Regexp above doesn't split apart the authority.
userinfo = authority[/^([^\[\]]*)@/, 1]
if userinfo != nil
user = userinfo.strip[/^([^:]*):?/, 1]
password = userinfo.strip[/:(.*)$/, 1]
end
host = authority.sub(
/^([^\[\]]*)@/, EMPTY_STR
).sub(
/:([^:@\[\]]*?)$/, EMPTY_STR
)
port = authority[/:([^:@\[\]]*?)$/, 1]
port = nil if port == EMPTY_STR
end

main

if authority != nil
# The Regexp above doesn't split apart the authority.
userinfo = authority[/^([^\[\]]*)@/, 1]
if userinfo != nil
user = userinfo.strip[/^([^:]*):?/, 1]
password = userinfo.strip[/:(.*)$/, 1]
end
host = authority.sub(
/^([^\[\]]*)@/, EMPTY_STR
).sub(
/:([^:@\[\]]*?)$/, EMPTY_STR
)
port = authority[/:([^:@\[\]]*?)$/, 1]
end
if port == EMPTY_STR
port = nil
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct. port is set to nil on line 133 before the block, so this check only needs to happen if the authority block changes the port value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a test to ensure proper functionality

@dentarg
Copy link
Collaborator

dentarg commented Dec 15, 2022

@dpep sorry letting this drag on... #486 changed things related to this PR. Does the changes here still make sense after #486? Feel free to rebase this PR and I'll have a look again

@dpep
Copy link
Contributor Author

dpep commented Dec 15, 2022

rebased. I think these changes still improve code quality / readability, but feel free to close

@dentarg dentarg merged commit eac45f9 into sporkmonger:main Dec 15, 2022
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

2 participants