dwyl / statuses

📝 A collection of statuses for use in our various apps.
GNU General Public License v2.0
5 stars 1 forks source link

Replace `File.read` used in package #4

Open SimonLab opened 2 years ago

SimonLab commented 2 years ago

I had a better look at the statuses code and I think we can make the dependency easier to read/use:

The following case is bothering me: https://github.com/dwyl/statuses/blob/f6783e0ea99cb7318e3b41d7bddfbb5dd83c0fef/lib/statuses.ex#L28-L39

So I proposed to refactor the package by:

@nelsonic let me know if this makes sense, I'll add also comments on my PR #3 to try to explain a bit more my reasoning

nelsonic commented 2 years ago

Hi @SimonLab, Apologies for delayed response. ⏳ While I totally agree that the File.read is brittle if the word "statuses" is in the file path, I would still prefer to have the JSON file because it's more versatile. e.g. when we want to use the statuses in JS or Dart/Flutter land, JSON is a lot easier.

As for renaming id to code, I can see how this would make sense in the context of "Status Codes" e.g: https://en.wikipedia.org/wiki/List_of_HTTP_status_codes

The update in #3 the change is "breaking" so technically, 1.0.1 -> 1.1.0 is not correct SemVer. it should be 2.0.0 However given that this package is only (currently) being used in one place: app-mvp-phoenix/priv/repo/seeds.exs#L19 ... happy to go with v1.1.0 Will just change the code in the MVP. 👍

nelsonic commented 2 years ago

Package published to https://hex.pm/packages/statuses/1.1.0 📦

nelsonic commented 2 years ago

The change from String to :atom on the status.text makes sense, but it creates more work. 💭

nelsonic commented 2 years ago
** (ArgumentError) errors were found at the given arguments:

  * 1st argument: not an atom

    :erlang.atom_to_binary("done", :utf8)
    priv/repo/seeds.exs:18: anonymous fn/1 in :elixir_compiler_1.__FILE__/1
    (elixir 1.13.4) lib/enum.ex:937: Enum."-each/2-lists^foreach/1-0-"/2
    priv/repo/seeds.exs:16: (file)

https://github.com/dwyl/statuses/blob/b3c5950bfbe5b14bec27f87e046e74a56e552586/lib/statuses.ex#L29

All other statuses are text: :atom except this one. 🙄

nelsonic commented 2 years ago

Remove get_json/0 and need for Jason dependency completely. ✂️ When we need JSON (e.g. for front-end offline-first app) we will figure it out. 👍

nelsonic commented 2 years ago

Having the status.text as :atom instead of String means we have to do a Atom.to_string(s.text) ...

nelsonic commented 2 years ago

The obvious disadvantage (IMO) of having the data embedded in the code:

https://github.com/dwyl/statuses/blob/242101885dad51502974c28a2e46f2ee01d52ef3/lib/statuses.ex#L11-L18

Is that now anyone contributing to the repo has to understand this new/weird data format. For a person who has never seen Elixir code, Struct is a lot less familiar than JSON. The chances that someone with basic JS, Python or even PHP skills has seen JSON is very high.

https://github.com/dwyl/statuses/blob/f6783e0ea99cb7318e3b41d7bddfbb5dd83c0fef/statuses.json#L2-L6

Not saying that the JSON version is better, it's not. Just that it's more familiar to most people. And then of course for the casual contributor to the @dwyl App who just wants to propose a new status, the GitHub editor will syntax highlight JSON better than a nested Struct. 👩‍💻 💭

Also, very curious what the performance benefit of the Struct is ...

P.S. we're definitely not switching this back to JSON; I just wanted to capture my thoughts.