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.

Sign up for GitHub

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

Jump to bottom

No part of a URL should be nil #78

Merged
merged 6 commits into from
Feb 3, 2016
Merged

No part of a URL should be nil #78

merged 6 commits into from
Feb 3, 2016

Conversation

roback
Copy link
Member

@roback roback commented Jan 29, 2016

An attempt to fix #52

@roback
Consider URLs without sld invalid
45a052c 
The list of URLs is taken from the comments in #52.
This does not fix #52 completely as Twingly::URL#trd still
could be nil.
@roback
Make sure no part of the url can be nil
d238a1a 
close #52

Only added test for #trd since all other methods are tested in the
"when given bad input" context.
@roback
Remove a "puts"
3ee2ab1 
This shouldn't have been committed :P
walro
walro reviewed Jan 29, 2016
View reviewed changes
lib/twingly/url.rb
@@ -38,6 +38,8 @@ def internal_parse(potential_url)
public_suffix_domain = PublicSuffix.parse(addressable_uri.display_uri.host)
raise Twingly::URL::Error::ParseError if public_suffix_domain.nil?

raise Twingly::URL::Error::ParseError unless public_suffix_domain.sld
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow suit with the if just above (change to if nil). :)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Didn't notice that

@walro
Copy link
Contributor

walro commented Jan 29, 2016

URLs without sld now results in a NullURL. (Im not sure whether my assumption here is correct, but it works for the URLs given as examples in #52)

I think I like this part.

Added .to_s to all getter methods (#scheme, #trd, etc.)

Not so sure about this part, one interpretation of nil is "it doesn't exist", where "" could be more like, it exists but it happens to be empty.

@roback
Copy link
Member Author

roback commented Jan 29, 2016

Not so sure about this part, one interpretation of nil is "it doesn't exist", where "" could be more like, it exists but it happens to be empty.

My thought was that since NullURL returns an empty string for all methods, so should URL.

@walro
Copy link
Contributor

walro commented Jan 29, 2016

My thought was that since NullURL returns an empty string for all methods, so should URL.

Yeah, and it might make sense here too. I think the calls to #to_s is what bothers me the most, in NullURLwe hide it :)

@roback
Copy link
Member Author

roback commented Jan 29, 2016

I think the calls to #to_s is what bothers me the most

I don't like them either, but at least its clear what is returned :)

@walro
Copy link
Contributor

walro commented Feb 1, 2016

Added .to_s to all getter methods (#scheme, #trd, etc.)

Does any of the urls added in the url_spec exercise this? Just looking the code it seems like the check for sld would pick them all up.

@roback
Copy link
Member Author

roback commented Feb 1, 2016

Added .to_s to all getter methods (#scheme, #trd, etc.)

Does any of the urls added in the url_spec exercise this? Just looking the code it seems like the check for sld would pick them all up.

No. I'm not sure how to test those methods with invalid urls since a NullURL is returned instead. I added a test for #trd though since that could be nil.

to_s isn't necessary on all methods, but now we have control over what gets returned instead of relying on public_suffix and addressable. (public_suffix returns nil for missing url components (sld, trdetc) while Addressable seems to prefer empty strings.)

@dentarg
Copy link
Collaborator

dentarg commented Feb 2, 2016

👍

@dentarg
Use #to_s only where needed
a4e618f 
As pointed out by @walro

>> Added .to_s to all getter methods (#scheme, #trd, etc.)

> Does any of the urls added in the url_spec exercise this? Just looking
the code it seems like the check for sld would pick them all up.

Let's try without the #to_s until we an counter-example.
@roback roback mentioned this pull request Feb 3, 2016
Merged
@roback
Copy link
Member Author

roback commented Feb 3, 2016

Should this be released as a patch version? I think it should, but the .to_s that was added to #trd is a breaking change, at least for norrstrom.

@dentarg
Copy link
Collaborator

dentarg commented Feb 3, 2016

I think we are already in for a new major, with the other changes that have been made in master (the private methods stuff).

On 3 feb. 2016, at 09:02, Mattias Roback notifications@github.com wrote:

Should this be released as a patch version? I think it should, but the .to_s that was added to #trd is a breaking change, at least for norrstrom.

—
Reply to this email directly or view it on GitHub.

@jage
Copy link
Contributor

jage commented Feb 3, 2016

Should this be released as a patch version? I think it should, but the .to_s that was added to #trd is a breaking change, at least for norrstrom.

Agree it should be a patch, but since we know it's a breaking change I think it's better to bump the major version. We didn't specify in any documentation that #trd should return empty string, so basically we changed the behavior now. It's hard to call this a "bug" to the users of the gem. :-)

@jage
Copy link
Contributor

jage commented Feb 3, 2016

Agree it should be a patch, but since we know it's a breaking change I think it's better to bump the major version. We didn't specify in any documentation that #trd should return empty string, so basically we changed the behavior now. It's hard to call this a "bug" to the users of the gem. :-)

If we first returned "", then by accident released a version that returned nil, this would have been a patch release IMO.

@roback
Copy link
Member Author

roback commented Feb 3, 2016

I think we are already in for a new major, with the other changes that have been made in master (the private methods stuff).

Wasn't aware of that.

We didn't specify in any documentation that #trd should return empty string, so basically we changed the behavior now.

Thats true, a new major version it is then :)

roback added a commit that referenced this pull request Feb 3, 2016
@roback
Merge pull request #78 from twingly/fix/52/domain-sld-trd-can-be-nil
bae9e5e 
No part of a URL should be nil
@roback roback merged commit bae9e5e into master Feb 3, 2016
@roback roback deleted the fix/52/domain-sld-trd-can-be-nil branch February 3, 2016 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

domain, sld, trd (maybe others) can be nil
4 participants
@roback @walro @dentarg @jage