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

Update PublicSuffix and Addressable #117

Merged
merged 1 commit into from
Feb 14, 2018

Conversation

roback
Copy link
Member

@roback roback commented Feb 9, 2018

Unfortunately it doesn't look like this fixes any of our issues, but since it made the profiling run a bit faster (and the fact that the tests didn't break) I made a PR of this anyway.

Profiling "total run time" went from 1.6663s to 1.4801s.

Some related links:

Unfortunately it doesn't look like this fixes any of our issues,
but since it made the profiling run a bit faster (and the fact that
the tests didn't break) I made a PR of this anyway.

(Profiling total run: 1.6663s -> 1.4801s).

Some related links:
* weppos/publicsuffix-ruby#130
* weppos/publicsuffix-ruby#133
* sporkmonger/addressable#267
@walro
Copy link
Contributor

walro commented Feb 9, 2018

We could take the opportunity and think about: #104 (comment)

My experience that going from loose requirements to strict ones made it a bit cumbersome for Bundler to figure out how to update. I would usually end up with twingly-url not being updated at all (unless I danced a bit). Not sure what I proposed, allowing patch versions, would be a silver bullet but it would likely help in some cases at least.

@roback
Copy link
Member Author

roback commented Feb 9, 2018

Just a note: We need at least Addressable 2.5.2, since that is the only version is compatible with public suffix 3.x: https://github.com/sporkmonger/addressable/blob/master/CHANGELOG.md#addressable-252

@walro
Copy link
Contributor

walro commented Feb 9, 2018

We could allow for something like: public_suffix >= 2, < 4 and something similar for addressable and let bundler figure out the rest. I am not really sure about how we properly test all combinations though.

@roback
Copy link
Member Author

roback commented Feb 9, 2018

I am not really sure about how we properly test all combinations though.

Looks like Travis already thought of that: https://docs.travis-ci.com/user/languages/ruby/#Testing-against-multiple-versions-of-dependencies, though we have to create the gemfiles manually.

@texpert
Copy link

texpert commented Feb 14, 2018

Pleading for a new release with this PR merged, because the google-api-client gem is long time requiring the 'addressable', '~> 2.5', '>= 2.5.1'

@dentarg
Copy link
Collaborator

dentarg commented Feb 14, 2018

Let's get this merged and released and then loosen up the dependencies and test with multiple gemfiles.

@dentarg dentarg merged commit 9d4a26a into master Feb 14, 2018
@dentarg dentarg deleted the update-addressable-and-public-suffix branch February 14, 2018 14:34
dentarg added a commit that referenced this pull request Feb 14, 2018
dentarg added a commit that referenced this pull request Feb 14, 2018
To release #117 and #118.
@dentarg
Copy link
Collaborator

dentarg commented Feb 14, 2018

@texpert done! see https://rubygems.org/gems/twingly-url/versions/5.1.1

@texpert
Copy link

texpert commented Feb 14, 2018

Awesome, @dentarg , thanks a lot!!

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

4 participants