elixir-maru / maru

Elixir RESTful Framework
https://maru.readme.io
BSD 3-Clause "New" or "Revised" License
1.32k stars 84 forks source link

Jason is optional dependency #97

Closed arkgil closed 6 years ago

arkgil commented 6 years ago

Hi!

I've noticed that in dependencies list Jason is marked as optional: https://github.com/elixir-maru/maru/blob/595e30736ea6f3e1fea2d9ef19cda8e5fa8912cc/mix.exs#L32

However, Jason is a default when fetching JSON library from application environment: https://github.com/elixir-maru/maru/blob/595e30736ea6f3e1fea2d9ef19cda8e5fa8912cc/lib/maru.ex#L62

If someone forgets to configure JSON library specifically, or doesn't add Jason to deps, undefined function errors show up because the module is not there.

I'd happily prepare a PR if that is something you're willing to accept 🙂

falood commented 6 years ago

Hi @arkgil ,

It seems it's a general way to do like this in elixir world, as far as I know, phoenix and ecto also do it like this.

https://github.com/elixir-ecto/ecto/blob/master/mix.exs#L54 https://github.com/phoenixframework/phoenix/blob/master/mix.exs#L60

arkgil commented 6 years ago

Yes, I'm aware that marking dependency as optional is a common practice. However, it's a little bit different in case of the two projects you've mentioned:

I'd say that Maru is mostly used for building JSON APIs (although I might be completely wrong here), so JSON library is essential in this kind of project. It would be nice if Maru would raise an explicit error if no such library is present 💥

Another, simpler solution is to just point out in the documentation that JSON library needs to be added to deps explicitly to be able to build a JSON API 📝 What do you think, @falood? 🙂

falood commented 6 years ago

thank you for your suggestion, I have make this pr https://github.com/elixir-maru/maru/pull/98 to resolve this, how do you think about it 🙂

arkgil commented 6 years ago

@falood Thanks! I've left one comment in the PR 😄