Enforce keyword syntax for tuples of size 2 inside list #52
Conversation
lib/ex_format.ex
Outdated
| @@ -416,7 +416,15 @@ defmodule ExFormat do | |||
|
|
|||
| # Tuple containers | |||
| def to_string({:{}, _, args} = ast, fun, state) do | |||
| tuple = "{" <> tuple_to_string(args, fun, state) <> "}" | |||
| inside_list? = Map.get(state, :inside_list?) | |||
There was a problem hiding this comment.
We expect the key to be in the state so let's go with state.inside_list? instead.
There was a problem hiding this comment.
Actually inside_list? is not always in state because I didn't add it as a default attribute when initializing the state map. That's why I have to use Map.get/2 which returns a nil if inside_list? is not found. Hence this explains why I have to do inside_list? == true, and why I didn't use the built-in syntax for map update. :P
Will add it as a default attribute to the state map during initialization if there's no objections.
There was a problem hiding this comment.
Add inside_list?: false when initializing the map, that's def the way to go yes.
lib/ex_format.ex
Outdated
| inside_list? = Map.get(state, :inside_list?) | ||
| tuple = | ||
| # Enforce keyword syntax for tuples of size 2 inside list | ||
| if inside_list? == true and length(args) == 2 do |
There was a problem hiding this comment.
inside_list? == true is the same as inside_list?.
lib/ex_format.ex
Outdated
| @@ -941,6 +949,7 @@ defmodule ExFormat do | |||
| end | |||
|
|
|||
| defp list_to_string(list, fun, state) do | |||
| state = Map.put(state, :inside_list?, true) | |||
| # Statsd metrics sink client | ||
| {:statix, "~> 1.0"}, | ||
| statix: "~> 1.0", |
There was a problem hiding this comment.
Oh boy this is an interesting one. We always write this as tuples for some reason :| Should we start writing them like this, which sucks because only works if you don't have options? I'm not sure how to proceed. \cc @josevalim
There was a problem hiding this comment.
Just curious, what could break if we start formatting mix.exs like this? 🤔
There was a problem hiding this comment.
I don't think anything would break, it'd just be annoying to have to rewrite everything using tuples when you wanted to add options.
There was a problem hiding this comment.
Ah I see, thanks @lpil. I feel that this is a minor compromise since you are getting a cleaner syntax in return. Safe to merge? @whatyouhide @josevalim
uohzxela
force-pushed
the
keyword-sugar-in-list
branch
from
a2c181b
to
55b6410
Compare
|
Not safe to merge as far as I'm concerned. I dont know how to fix it but I
would try and think of a solution first.
On Fri, 4 Aug 2017 at 02:00, Alex Jiao ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In test/unit/list_test.exs
<#52 (comment)>:
> # Statsd metrics sink client
- {:statix, "~> 1.0"},
+ statix: "~> 1.0",
Ah I see, thanks @lpil <https://github.com/lpil>. I feel that this is a
minor compromise since you are getting a cleaner syntax in return. Safe to
merge? @whatyouhide <https://github.com/whatyouhide> @josevalim
<https://github.com/josevalim>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#52 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADtcSmMV0B8-RNQY5HOCtj3HoBQGUsUmks5sUl8jgaJpZM4Orynw>
.
--
Andrea Leopardi
an.leopardi@gmail.com
|
|
@uohzxela so me and @josevalim were talking about this and figured out that a way to move this forward for now is to respect what the user wrote wrt tuples vs keyword syntax. I believe most of the time people write keyword syntax anyways so it shouldn't really be a problem. |
This fixes issue #44. As discussed, issue #49 is a related problem but requires a different solution, so I will open a separate PR for that.