Closed kination closed 7 years ago
It looks like Travis is choking due to being configured with old Elixir versions. We should probably consider dropping the test cases for these and adding new ones to match Plug's mix.exs
(so Elixirs "1.2.3" and "1.3.something", plus "1.4.0"), unless there's some specific demand to support pre-1.2 Elixir versions.
I reckon we should also consider dropping Ecto as an explicit dependency; AFAIK, there's nothing particularly dependent on Ecto in Sugar itself (I mean, the ensure_authenticated
plug was designed around it, but it should work perfectly fine with any old function that provides any old struct, and if needed I'd be happy to rework that to be more generic).
@slogsdon: thoughts?
That sounds good. We can convert the Exto dependency to be optional if necessary. I can correct the Travis config soon to get that sorted out.
On Wed, Jan 25, 2017 at 6:41 PM -0500, "Ryan S. Northrup" notifications@github.com wrote:
It looks like Travis is choking due to being configured with old Elixir versions. We should probably consider dropping the test cases for these and adding new ones to match Plug's mix.exs (so Elixirs "1.2.3" and "1.3.something", plus "1.4.0"), unless there's some specific demand to support pre-1.2 Elixir versions.
I reckon we should also consider dropping Ecto as an explicit dependency; AFAIK, there's nothing particularly dependent on Ecto in Sugar itself (I mean, the ensure_authenticated plug was designed around it, but it should work perfectly fine with any old function that provides any old struct, and if needed I'd be happy to rework that to be more generic).
@slogsdon: thoughts?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
@slogsdon @YellowApple Thanks. It seems good to make Ecto as an explicit dependency to make project more modular.
mix test
seems work well on my laptop. Would it be problem of elixir version. If it is, is there a problem to change elixir target version in travis?
@slogsdon @YellowApple If you are busy, would you mind I work on update of this case?
Thanks.
Sorry. Yeah, go for it if you want to tackle it in this PR (else, I'll hack away at it sometime tomorrow).
@YellowApple Oh thanks! I'll look on yours and see what I can do more.
Gah; try to make Travis happy and you just annoy Coveralls ;)
Summarizing the changes I've pushed to the PR to reflect previous discussion:
Sugar.Config.get
tweaked to only merge default options into keyword lists (since Keyword.merge
in Elixir 1.4.0 is stricter re: which lists are eligible for merging); corresponding test cases updated as wellAll tests currently passing on all supported versions of Elixir, and coverage drop is effectively insignificant AFAICT. Given the scope of this change, pinging @slogsdon for review and approval (or rejection, of course!) before I merge this in and publish to Hex.pm.
Great~thanks.
Merged. Will push up to Hex.pm shortly.
Published. Thanks to @djKooks for the PR and @slogsdon for the review.
There are some warnings which causing from old version of http_router. Test case clears well. Please look on.