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

Upgrade poison to 3.0 #35

Merged
merged 3 commits into from
Nov 19, 2017

Conversation

vyorkin
Copy link
Contributor

@vyorkin vyorkin commented Jul 17, 2017

same as #16 , but bumps poison to 3.0 and has no conflicts

@coveralls
Copy link

coveralls commented Jul 17, 2017

Coverage Status

Coverage decreased (-0.6%) to 54.717% when pulling f9fd856 on vyorkin-forks:feature/upgrade-poison into ff08e3c on knrz:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 54.717% when pulling f9fd856 on vyorkin-forks:feature/upgrade-poison into ff08e3c on knrz:master.

@vyorkin
Copy link
Contributor Author

vyorkin commented Jul 17, 2017

how can it be decreased if I've only changed mix.exs and mix.lock?

@carpodaster
Copy link
Collaborator

Thanks @vyorkin!

This removes compatibility for versions 1.x and 2.x of poison which I'd rather avoid. Sadly, poison doesn't include a CHANGELOG to see what justified their major version bump (normally used for backward incompatible changes in Semver).

Can you change the dependency so that all versions are allowed?

@vyorkin
Copy link
Contributor Author

vyorkin commented Jul 18, 2017

just to be clear, you want all versions to be allowed {:poison, ">= 0.0.0"} or just {:poison, ">= 1.5.0 and < 4.0.0"} ?

{:towel, "~> 0.2"},
{:poolboy, "~> 1.5"},
{:geohash, "~> 0.1"},
{:ex_doc, ">= 0.0.0", only: :dev},
{:inch_ex, only: :docs},
{:inch_ex, ">= 0.0.0", only: :docs},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

had to change this to suppress warning

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

mix.exs Outdated
@@ -27,12 +27,12 @@ defmodule Geocoder.Mixfile do

defp deps do
[{:httpoison, "~> 0.8"},
{:poison, "~> 3.0"},
{:poison, ">= 0.0.0"},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume you want to completely relax the dependency

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, @vyorkin, for not getting back to you earlier. Coming back to your question further up, I think I would prefer < 4.0.0 in case poison ever starts to submit a changelog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, will update in a minute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@coveralls
Copy link

Coverage Status

Coverage remained the same at 55.346% when pulling f295ee0 on vyorkin-forks:feature/upgrade-poison into ff08e3c on knrz:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 55.346% when pulling f295ee0 on vyorkin-forks:feature/upgrade-poison into ff08e3c on knrz:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 55.346% when pulling f295ee0 on vyorkin-forks:feature/upgrade-poison into ff08e3c on knrz:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 55.346% when pulling f295ee0 on vyorkin-forks:feature/upgrade-poison into ff08e3c on knrz:master.

Copy link
Collaborator

@carpodaster carpodaster left a comment

Choose a reason for hiding this comment

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

If you could restrict the poison dep to < 4.0.0, we're good to go!

@coveralls
Copy link

coveralls commented Nov 19, 2017

Coverage Status

Coverage remained the same at 55.346% when pulling 5a7b49f on vyorkin-forks:feature/upgrade-poison into ff08e3c on knrz:master.

@carpodaster carpodaster merged commit 8ae1763 into CyrusOfEden:master Nov 19, 2017
@carpodaster
Copy link
Collaborator

Thank you, @vyorkin !

@vyorkin vyorkin deleted the feature/upgrade-poison branch November 19, 2017 22:20
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.

3 participants