Skip to content
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

Replace File.read used in package #4

Open
3 tasks done
SimonLab opened this issue Jul 8, 2022 · 7 comments
Open
3 tasks done

Replace File.read used in package #4

SimonLab opened this issue Jul 8, 2022 · 7 comments
Labels
enhancement New feature or request

Comments

@SimonLab
Copy link
Member

SimonLab commented Jul 8, 2022

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:

statuses/lib/statuses.ex

Lines 28 to 39 in f6783e0

case cwd =~ "/statuses" do
true ->
read_decode()
# coveralls-ignore-start
false -> # temporarily cd into deps/quotes dir and read quotes.json file:
File.cd!("deps/statuses")
data = read_decode()
# chnge back into the root directory
File.cd!("../..")
data # return the decoded (parsed) JSON data
# coveralls-ignore-stop
end

  • This case is here to be able to distinguish when the code is run in test and when it is used in another application as a dependency, however it took me a bit of time to understand fully this logic and add to log the File.cwd information to check the value on an application using the dependency

  • We can have an edge case where the application using the dependency has a file path containing /statuses, in this case the cwd =~ "/statuses" will be true, however the status.json file will not be found by the application.

  • We have created the statuses.json that we then read and convert to a list of map in Elixir. Because the dependency is only used with other Elixir application I think we can create directly the list of status map without using json. This would resolve the first point above as we won't have to use File.read and make sure the file path is the correct one. The main reason to have a json file is to make it easy to read and update the status, however I feel an Elixir list of map is as readable and can easily be updated, if really required we can add a small description on how to do this on the Readme.

So I proposed to refactor the package by:

  • Removing the json file
  • Creating the list of statuses directly in the Statuses module
  • Renaming the id to code to decouple the status code from the Postgres library and letting Ecto manage the link between status and the application items.

@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

@SimonLab SimonLab added the enhancement New feature or request label Jul 8, 2022
@nelsonic
Copy link
Member

nelsonic commented Jul 9, 2022

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
Copy link
Member

nelsonic commented Jul 9, 2022

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

nelsonic added a commit to dwyl/mvp that referenced this issue Jul 9, 2022
@nelsonic
Copy link
Member

nelsonic commented Jul 9, 2022

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

@nelsonic
Copy link
Member

nelsonic commented Jul 9, 2022

** (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)

%Status{code: 4, desc: "Items marked as done are complete", text: "done"},

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

@nelsonic
Copy link
Member

nelsonic commented Jul 9, 2022

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
Copy link
Member

nelsonic commented Jul 9, 2022

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

@nelsonic
Copy link
Member

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

statuses/lib/statuses.ex

Lines 11 to 18 in 2421018

def get_statuses() do
[
%Status{
code: 1,
desc:
"A person verified by a 3rd party OAuth provider or by confirming their email address",
text: :verified
},

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.

{
"text": "verified",
"id": "1",
"desc": "A person verified by a 3rd party OAuth provider or by confirming their email address"
},

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants