-
Notifications
You must be signed in to change notification settings - Fork 96
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
Updated dependencies, formatting, dialyzer, credo #63
base: master
Are you sure you want to change the base?
Conversation
@Azolo Can you take a look, please? |
Sorry, am out of town. There's a lot of changes here, so I will as soon as I get back. |
@@ -0,0 +1,2 @@ | |||
erlang 21.1 | |||
elixir 1.7.3-otp-21 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this project supports Elixir 1.3. Is this a generated file included by mistake?
@InoMurko a bit of feedback to help make PRs easier to review (I general, I am not a maintainer of this project). It's best, if you intend to run the code formatter as part of your patch, to do it in a separate commit from something like updating dependencies. It ends up making a large and noisy diff for review, and also looks confusing when looking at git history. Additionally, it creates a larger surface area for unnecessary merge conflicts. I'm not sure if this would be an issue getting this particular patch in, but taking the code formatting out and making a different PR for it could help move it through more quickly. |
@mmartinson you're perfectly right. I first made the changes for myself and after that decided to push it upstream - hence the merger with formatting. If it's an issue we can't get pass I can rework that. There's a dependency reason why I decided to deprecate old Elixir versions, not exactly sure which one but I can figure it out next week! |
It looks like exdoc 0.19 requires 1.7, but 0.18 will support back to 1.3. I find it tricky to determine how many previous language versions are worth supporting for a given library in general, but for a newer one like this that likely doesn't have a couple years being adopted to production, I think supporting 1 trailing version is reasonable. With that, I would suggest changing exdoc to 0.18, and requiring >= Elixir 1.6 in mix.exs |
That makes sense and it should be an easy change. Thanks for your input! |
This. If they were smaller changes or changes I could glance over and merge then I would. Thanks @mmartinson for the help. |
Ok. I’ll change this over the weekend or next week! |
Removed all associations to the request object since it's been deprecated in latest cowboy.