Pretty please don’t ever add 2 newlines to the formatter 
def foo(1), do: "foo"
def foo(x), do: x
def bar(x), do: x * 2
If you can’t tell there are two function heads there by reading, maybe add a comment or typespec.
1 Like
Okay, a slightly more subjective one here, but:
%Paginator{
path: path,
size: size,
pagers: pagers(path, size, chunks),
pages: []}
is formatted by the formatter as
%Paginator{path: path, size: size, pagers: pagers(path, size, chunks), pages: []}
The struct does fit within a single line’s character count, but while you can easily scan downward on the former and see what the fields are, you have to scan across both fields and values to see all the fields of the latter.
On the other hand it saves a bunch of lines, so… ???
2 Likes
That’s a very good question! I believe I have already made those points, and I am glad to repeat them, I just don’t want us to go on a loop. 
So here we go:
The ambiguity part was about respecting the user’s choice in the formatter. The formatter, as well as a human reader, can’t distinguish between a mistake or user’s choice in some of my examples above.
The inconsistency part is about folks that want to respect the style in a codebase when working on it. As well as the team receiving contributions also want those contributions to align with their source code. This can be a new person joining the company, someone contributing to OSS, etc. If you have loose rules based on our perception of readability, then the person will have difficulty in figuring out the proper rules or the project will go into a direction where each person uses their own style. But maybe those styles are actually really close to each other and then it is not a problem.
Point taken. It should not be a surprise that we have put a lot of thought into it already. But it definitely doesn’t hurt to put more. From my side, I just ask you to remember that there are cons with respecting the user’s choice.
EDIT: Just as a joke, but maybe we didn’t make it strict enough. Because we do respect the user’s choice in some occasions. If we had made it fully strict and then added those cases then everyone would be much happier now with the flexibility they got. 
2 Likes
@mischov this is covered in the formatter docs. 
The formatter will respect your choice if you add a new line after { and before }, like this:
%Paginator{
path: path,
size: size,
pagers: pagers(path, size, chunks),
pages: []
}
In other words, we keep it multiline if you write it in the same multiline way as the formatter would.
4 Likes
Ah, of course- it’s missing the newline before }! I was wondering why the formatter compressed it anyway.
Would it make sense to keep it multiline if you have just the newline after { but not before } (automatically adding the newline before })?
Edit: Wow, the formatter source is complicated. I haven’t yet been able figure out how the formatter decides whether to make a map multiline or not! Any good primer on understanding how the formatter works?
Second Edit: I guess having a newline after { but not before } wouldn’t add :eol to the quoted map metadata, and therefore not trigger the multiline case in args_to_algebra_with_comments? After looking through all that I think I am happy with the current behavior,
1 Like
Apologies if this is discussed above or elsewhere, couldn’t find*, I know the .formatter.exs will allow one of the VERY few customizations via locals_without_parens, but will mix packages be able to, perhaps, have their .formatter.exs files automatically merged in to a requiring project’s .formatter.exs? When we ran this on our work project, all the Ecto macro’s suddenly had parentheses, and while macro’s we write ourselves and want to be parens-less clearly need adding, if the goal is consistenty, it seems it should allow consistent use of macros exported from libraries, and standard library macros, to all be on similar/equal footing.
(*and the site takes over browser ctrl-f so not even sure from searching page)
Oh, hmm, I had this link open https://github.com/elixir-lang/elixir/issues/6644 but hadn’t read yet, sounds like it’s talking about same thing, but I still think allowing a hex package to export/merge in it’s locals_without_parens, either the same one they use for developing/formatting their own code, or a secondary external_without_parens parameter or something… it seems not too different from auto-starting applications from dependencies to also auto-import, or import-by-default, whatever little configurability is allowed to users via the pacakge system.
It is worth mentioning that Ecto’s .formatter.exs is for Ecto’s private usage and may contain locals that only make sense for Ecto. For example, see Elixir’s config file where all entries are relevant to Elixir tests only.
In any case, we may still support a similar feature to the one you are asking, although you will have to explicitly say it:
%{
load_locals_from: "deps/ecto/public_formatter.exs"
}
or something of sorts. The formatter has to be super fast so editors can format files and provide a fast feedback cycle. Therefore any kind of “automatically figuring out of things” may add unnecessary overhead.
But for now the plan is that Phoenix will generate a .formatter.exs file into your application that has all of the relevant macros listed.
1 Like
Why phoenix only? Ecto can be used standalone very well.
Therefore we need something that “just” works, also it needs to feel official right from the beginning:
So whats about a recursive mix task that goes through dependencies and collects their formatting rules (another mix task? Add a config to mix which specifies a file to copy or a function to call?)? Then those could even derive from the project specific locals and it were a one time thing (after deps have been added), also one only needs to parse and consume a single file.
Eg. on mix fmt.fetch_locals the formatter asks each dependency for their suggestions, it will return a simple list of rules, those lists of all deps will get concatenated, and written into another file. .formatter.exs and .external_formatters.exs will then be merged on a mix fmt run (er even merged once into a third file which is only regenerated when one of the other 2 chages).
That’s a good idea and we probably don’t need separate files. Phoenix “.formatter.exs” can provide an “export” field and the user “.formatter.exs” can have an “import” where each entry is a dependency and their imported configuration. It should be ok for the tool to work on the user’s .formatter.exs since we can format it before writing it to disk.
The only issue I see with this approach is that “.formatter.exs”, based on its name, does not sound like a file that should be part of packages and we will also need to change hex default instructions to include it.
I do not talk about packaging the .formatter.exs, since as you said, its local to the package, I’m talking about adding a field to *.MixFile.project which resolves to the external formatter config of a project.
This way, we had a clear distinction between the configuration used for developing in a project and that configuration that is used when using the project as a dependency.
edit
Also, I think only config of direct dependencies should be pulled in, not transient ones. If we simply collect all .formatter.exs and merge them, then we can’t do anything against transient dependencies.
There are two things: we don’t want projects to have many formatter related files. So between adding a .formatter.exs or .external_formatter.exs to source control, I would rather keep the first and have a single file.
And the second is that loading and parsing the Mixfile.project is going to add a bunch of indirection if all we want to load the formatter options. Especially because the formatter was designed to work without a project.
Yes but then you have things like the phoenix_ecto project which exists to manage the ecto dependencies for you in regards to phoenix. Maybe the best way to go here is to be opt-in?
mix format.import_deps ecto phoenix
In my suggestion, only .formatter.exs is added to the VCS, the external one can get regenerated.
You don’t do it when loading the generator options, but when deps.getting or deps.compileing. From the gathered information you then generate the external file.
Thats indeed a problem. But then again, those projects could publish the rules of their direct dependencies explicitely. And since the API to fetch those is specified, it can be even done programmatically.
I am afraid we are talking about different things then. There are two projects here: the parent project and the dependency project. I am talking about the latter.
The dependency needs to specify the configuration it wants to push to its parent. If we decide it is done outside of the mix.exs file, then regardless if it is .formatter.exs or .external_formatter.exs, those files will require changes in Hex so they are included by default. See my comment above about why we may want that to be outside mix.exs.
I really don’t think we should be merging formatter configuration and writing to the project root when compiling dependencies…
It also means that users have little control. What if project X that you depend directly has actually a formatting that your team is really uncomfortable with? Elixir is generally against allowing your dependencies to change how your project runs and it is usually explicit. It is very likely that a team will agree on bringing the configuration of only certain dependencies and this choice needs to be stored on version control.
1 Like
I see your points. Thank you for your explanation.
1 Like
Glad to help! In any case, I really like the idea of having a step (be it automatic or manual) where we get the configuration from the dependencies, since it solves my concern slowing down the formatter. So thanks for the suggestion!
1 Like
If you hit <ctrl>+f once it opens an in-site search to search the entire thread (which may not even be loaded into the browser memory yet, hence why the normal <ctrl>+f would not work), however hitting <ctrl>+f a second time closes the in-site one and opens the normal browser’s find menu so you can search the already loaded content.
If you are searching for something in the current thread but do not want to preload the entire page (say by hitting pgdn, then holding pgup to get to the top when it all loads) then the in-site one is better to use.
4 Likes
[quote=“OvermindDL1, post:98, topic:9344”]
(say by hitting pgdn, then holding pgup to get to the top when it all loads)[/quote]
Which may fail, since discourse will unload parts of the DOM on large threads…
1 Like
TIL! I needed this so many times.
@mischov the formatting bug you have reported is now fixed. Please recompile Elixir from master and run the formatter again on your project and let us know how it goes since the fix may now reveal other cases.
4 Likes
The tuples look much better!
The Good
Since I seem to only be mentioning problems I wanted to take a moment and acknowledge a really useful bit of formatting I saw going through stuff.
defp parse_attribute([{:ident, attr}, type, {:ident, val}, ']' | toks]) when type in @attribute_value_selector_types do
...
end
formats to
defp parse_attribute([{:ident, attr}, type, {:ident, val}, ']' | toks])
when type in @attribute_value_selector_types do
...
end
The Bad
Okay, “bad” is probably an overstatement, but I was a bit surprised about this one (because it has a newline after ( and before )).
compare(
expr.op,
Expr.Helpers.eq_fmt(v1, v2, document),
Expr.Helpers.eq_fmt(v2, v1, document)
)
formats to
compare(expr.op, Expr.Helpers.eq_fmt(v1, v2, document), Expr.Helpers.eq_fmt(v2, v1, document))
The Ugly
Not sure there is much that can be done about this one, but the enormous tail of closing delimiters make me shake my head a little.
selectors = [
%Element{
selectors: [
%Element.Tag{value: "tag"},
%Element.PseudoClass.Not{
args: [
[%Element{
selectors: [
%Element.Tag{value: "div"},
%Element.Attribute.ValueIncludes{attribute: "class",
value: "class"}]}]]}]}]
formats to
selectors = [
%Element{
selectors: [
%Element.Tag{value: "tag"},
%Element.PseudoClass.Not{
args: [
[
%Element{
selectors: [
%Element.Tag{value: "div"},
%Element.Attribute.ValueIncludes{attribute: "class", value: "class"}
]
}
]
]
}
]
}
]
