search

Azolo / websockex

An Elixir Websocket Client
MIT License
515 stars 97 forks source link

Updated dependencies, formatting, dialyzer, credo #63

Open InoMurko opened 5 years ago

InoMurko commented 5 years ago

Removed all associations to the request object since it's been deprecated in latest cowboy.

ayrat555 commented 5 years ago

@Azolo Can you take a look, please?

Azolo commented 5 years ago

Sorry, am out of town. There's a lot of changes here, so I will as soon as I get back.

mmartinson commented 5 years ago

@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.

InoMurko commented 5 years ago

@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!

mmartinson commented 5 years ago

It looks like exdoc 0.19 requires 1.7, but 0.18 will support back to 1.3. plug_cowboy 2.0 requires 1.5 dialyxir 1.0-rcx requires 1.6 Not sure if I missed any

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

InoMurko commented 5 years ago

That makes sense and it should be an easy change. Thanks for your input!

Azolo commented 5 years ago

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.

This.

If they were smaller changes or changes I could glance over and merge then I would. But right now going through it all is a time commitment I don't have.

Thanks @mmartinson for the help.

InoMurko commented 5 years ago

Ok. I’ll change this over the weekend or next week!