Issue175

[kbaird]

Makes FormatRequired plug accept a legal multi-RIO payload.

[kbaird]

Technically, DELETE is handled earlier by the first clause. But it is one of the allowed methods for the multi-RIO payload, so I thought including it here made more sense than omitting it.

[jherdman]

The Credo changes are a bit surprising. On my dev machine, and CI, mix credo reports no concerns. Would you mind discussing your set up? Perhaps it'll help us figure out the discrepancies.

[kbaird]

Sure. Here's what I get in master.

$ iex -v
Erlang/OTP 21 [erts-10.2.3] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:1] [hipe]

IEx 1.8.1 (compiled with Erlang/OTP 20)

$ mix credo --strict
Ignoring an undefined check: Credo.Check.Warning.NameRedeclarationByAssignment
Ignoring an undefined check: Credo.Check.Warning.NameRedeclarationByCase
Ignoring an undefined check: Credo.Check.Warning.NameRedeclarationByDef
Ignoring an undefined check: Credo.Check.Warning.NameRedeclarationByFn
Checking 17 source files ...

info: the following checks were skipped because they're not compatible with
your version of Elixir (1.8.1).

   1) Credo.Check.Refactor.MapInto


  Code Readability                                                              
┃ 
┃ [R] ↘ The alias `JSONAPI.Config` is not alphabetically ordered among its 
┃       group.
┃       lib/jsonapi/serializer.ex:9:9 #(JSONAPI.Serializer)
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃       lib/jsonapi.ex:33 #(JSONAPI.mime_type)
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃       lib/jsonapi.ex:12 #(JSONAPI.json_library)

Please report incorrect results: https://github.com/rrrene/credo/issues

Analysis took 0.4 seconds (0.01s to load, 0.4s running checks)
168 mods/funs, found 3 code readability issues.

[jherdman]

OH! Gotcha. OK, AFAIK we're not using the --strict flag. Would you mind reverting the Credo changes for now? If we want to move to strict that may be a worthy PR in-and-of-itself.

[kbaird]

Credo changes now in #177

[jherdman]

@kbaird could you rebase this against master? It has your Credo changes, and I want to make sure the build is still green (I'm sure it is, but ya know... safe vs. sorry).

[jherdman]

LGTM but I'd love to see a rebase before merging. I'd feel better about the CI results.

[kbaird]

Rebased via the procedure at https://gist.github.com/ravibhure/a7e0918ff4937c9ea1c456698dcd58aa

[doomspork]

@kbaird something isn't quite right but I think it's c1b1849 that's the culprit, it adds () to the functions and fails Credo.

There's 2 ways we can fix this:

$ git fetch --all --prune
$ git rebase upstream/master -i --autostash --autosquash
# I do interactive rebase so I can squash, fixup, and rename commits.
# At the screen that pops up, delete the c1b1849 line, save and exit.
# That should remove that commit for us.
$ git push origin master -f

You could add a new commit that undoes those changes, when we squash it won't matter 😁

I'd be happy to help you if that doesn't resolve the issues. I'd expect the credo --strict call to pass on your branch.

[kbaird]

@doomspork I just backed it up one commit. Let me know if it needs any more attention from me. Thanks.

[jherdman]

@doomspork you good with this? I think we're all set to merge.

[snewcomer]

👍 ⚡️

[doomspork]

Thanks so much @kbaird and @jherdman 😁