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

Respect ribbon for function call as function argument #188

Open
paulo-ferraz-oliveira opened this issue Oct 7, 2020 · 3 comments
Open
Labels
configuration idea style change Improvements on a formatter style

Comments

@paulo-ferraz-oliveira
Copy link
Contributor

paulo-ferraz-oliveira commented Oct 7, 2020

Currently, default_formatter formats this code…

    IndexedIpAddresses = lists:zip(lists:seq(1, NIpAddresses), IpAddresses),

…like this…

    IndexedIpAddresses =
        lists:zip(
            lists:seq(1, NIpAddresses), IpAddresses),

I would like it to be formatted like this…

    IndexedIpAddresses =
        lists:zip(lists:seq(1, NIpAddresses), IpAddresses),

(especially since it's not going over ribbon)

@paulo-ferraz-oliveira paulo-ferraz-oliveira added configuration idea style change Improvements on a formatter style labels Oct 7, 2020
@elbrujohalcon
Copy link
Collaborator

Sadly… it's practically impossible, at least without replacing prettypr as our pretty-printer.
I'll try to give you the best explanation I can come up with…

prettypr expects a group of "related" documents as its input. Those documents can be paragraphs, sequences, sep (which is a kind of paragraph… sorta), etc… Then it places them in the best way it sees fit (respecting paper and ribbon whenever possible).

The behaviour that you (and to be fair, I… as well) expect from these things is akin to the paragraph behavior in the sense that we want prettypr to put as many function calls in a row as possible and only apply indentation when needed.
Therefore, in prettypr terms, we want a paragraph with the words IndexedIpAddresses =, lists:zip(, lists:seq(, 1,, … etc…

And prettypr can totally understand that, but what it cannot understand is that we want those words printed without spaces between them… IndexedIpAddresses = lists:zip(lists:seq(1,…. That simply makes no sense to prettypr and therefore the best thing it can do is… IndexedIpAddresses = lists:zip( lists:seq( 1, … (notice the spaces after ().

When I was working on #153, that was my first implementation. But I noticed it would not work like that.
So, there is an alternative (which is what the erl_prettypr from OTP or otp_formatter here use) which is to put all the things we want to be together in a single "word" (lists:zip(lists:seq(). That works relatively well when you don't compose too many functions, but it is problematic on these examples (which led to #140)…

    eyer_hosted_match:cluster_id(eyer_hosted_match:canonical_id(eyer_hosted_match:new([{id,
                                                                                        HashKey}]),
                                                                attr(?CANONICAL_ID,
                                                                     AttrMap,
                                                                     undefined)),
                                 attr(?CLUSTER_ID, AttrMap, undefined))

The third alternative, which is the default one now (but guarded behind the flag I introduced in #172) is to explicitly tell prettypr that some "words" must close some paragraphs and that the next thing is in a different paragraph or sequence, that must be indented to some degree. In this case, my idea was to do that with long function names, and only when two of those are composed. But then again… there is no real concept of long function names for the formatter. So I had to come up with a heuristic, which was qualified function names. If you compose two fully qualified function calls, the second one goes into a new paragraph that is indented below the one used for the first qualified function call. That's #153, basically.

It's not ideal and it may lead to some unexpected, undesired, or seeming inconsistent behavior.
On @AdRoll's code, it looks noticeably better than not using it (we probably have a tendency to compose long function names with long module names, etc…) and that's why it's the default. But you can totally disable that behavior, using inline_qualified_function_composition => false wherever you need to.

In any case, if you would like to take a deep dive into the formatter and prettypr modules… And if you find out a better way to produce the proper formatter… I would be delighted to merge such a PR.

@elbrujohalcon
Copy link
Collaborator

I added a link to this thread in the README, by the way.

@elbrujohalcon elbrujohalcon modified the milestones: 1.0.0, Eventually 🙄 Oct 7, 2020
@elbrujohalcon
Copy link
Collaborator

This is related to #199.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration idea style change Improvements on a formatter style
Projects
None yet
Development

No branches or pull requests

2 participants