Operators "before" items

[elbrujohalcon]

Currently, default_formatter formats this code…

-type x() :: {a, very, long, thing} | {another, very, long, thing} | [yet | another | very | long | thing].

-spec x() -> {a, very, long, thing} | {another, very, long, thing} | [yet | another | very | long | thing].
x() -> {a, very, long, thing} orelse {another, very, long, thing} orelse [yet , another , very, long , thing].

…like this…

-type x() :: {a, very, long, thing} |
             {another, very, long, thing} |
             [yet | another | very | long | thing].

-spec x() ->
           {a, very, long, thing} |
           {another, very, long, thing} |
           [yet | another | very | long | thing].
x() ->
    {a, very, long, thing} orelse
        {another, very, long, thing} orelse [yet, another, very, long, thing].

I would like to have an option for it to be formatted like this…

-type x() :: {a, very, long, thing}
           | {another, very, long, thing}
           | [yet | another | very | long | thing].

-spec x() ->
          {a, very, long, thing}
        | {another, very, long, thing}
        | [yet | another | very | long | thing].
x() ->
    {a, very, long, thing}
    orelse {another, very, long, thing}
    orelse [yet, another, very, long, thing].

This might be just part of the infamous rok_formatter, if it's too hard to make it an option for the default_formatter without breaking everything.

This was requested by @okeuday, @paulo-ferraz-oliveira, and others on WhatsApp/erlfmt#167 and other places.

[paulo-ferraz-oliveira]

it's too hard to make it an option

But isn't the point of options to change a default (previously established) behaviour?

[elbrujohalcon]

I meant… _if it's too hard to write the code to support such an option within default_formatter_ ;)

[paulo-ferraz-oliveira]

My preference: these get put in the next line (when a change is in order - I imagine this happen mostly for lack of horizontal space):

• https://erlang.org/doc/reference_manual/expressions.html#term-comparisons,
• https://erlang.org/doc/reference_manual/expressions.html#arithmetic-expressions,
• and, andalso, or, orelse, xor, |, ||, ++, <-, ->, =,
• [not commas, since this'll take some time getting used to 😄]. (I might be forgetting something)

[paulo-ferraz-oliveira]

If you think it possible, I'm willing to help with this one.

[okeuday]

I am not attempting to say you shouldn't implement the approach described above. However, I have found it necessary to always use the indent size on the next line in an effort to keep code into 80 columns. I understand go and IDE development prefers to not care about columns and your formatter may be avoiding the concern. With record type specs it has helped to indent on the next line with the next line starting with ::. The same with when for function guards.

Examples:

-type period_seconds() ::
    1..(?TIMEOUT_MAX_ERLANG div 1000).
-export_type([period_seconds/0]).

-type aspect_init_after_internal_f() ::
    fun((Args :: list(),
         Prefix :: cloudi_service:service_name_pattern(),
         Timeout :: timeout_initialize_value_milliseconds(),
         State :: any(),
         Dispatcher :: cloudi_service:dispatcher()) ->
        {ok, StateNew :: any()} |
        {stop, Reason :: any(), StateNew :: any()}).
-type aspect_init_after_external_f() ::
    fun((CommandLine :: list(string()),
         Prefix :: cloudi:service_name_pattern(),
         Timeout :: timeout_initialize_value_milliseconds(),
         State :: any()) ->
        {ok, StateNew :: any()} |
        {stop, Reason :: any(), StateNew :: any()}).

https://github.com/CloudI/CloudI/blob/025d3cf70e9cc0f9ef8ab593cb7c6250684c5467/src/lib/cloudi_core/src/cloudi_service_api.erl#L183-L201

-record(config_logging_syslog,
    {
        identity = "CloudI"
            :: cloudi_service_api:logging_syslog_identity(),
        facility = local0
            :: cloudi_service_api:logging_syslog_facility(),
        % The mapping for CloudI levels to syslog priorities is:
        % fatal  -> critical       (2)
        % error  -> error          (3)
        % warn   -> warning        (4)
        % info   -> notice         (5)
        % debug  -> informational  (6)
        % trace  -> debug          (7)
        level = trace
            :: cloudi_service_api:loglevel(),
        transport = local
            :: cloudi_service_api:logging_syslog_transport(),
        transport_options = []
            :: cloudi_service_api:logging_syslog_transport_options(),
        protocol = rfc3164
            :: cloudi_service_api:logging_syslog_protocol(),
        path = "/dev/log"
            :: cloudi_service_api:logging_syslog_path(),
        host = {127,0,0,1}
            :: cloudi_service_api:logging_syslog_host(),
        port = undefined
            :: cloudi_service_api:logging_syslog_port()
    }).

https://github.com/CloudI/CloudI/blob/025d3cf70e9cc0f9ef8ab593cb7c6250684c5467/src/lib/cloudi_core/src/cloudi_core_i_configuration.hrl#L44-L71

acl_add([_ | _] = L, Timeout)
    when ((is_integer(Timeout) andalso
           (Timeout >= ?TIMEOUT_SERVICE_API_MIN) andalso
           (Timeout =< ?TIMEOUT_SERVICE_API_MAX)) orelse
          (Timeout =:= infinity)) ->
    cloudi_core_i_configurator:acl_add(L, Timeout).

https://github.com/CloudI/CloudI/blob/025d3cf70e9cc0f9ef8ab593cb7c6250684c5467/src/lib/cloudi_core/src/cloudi_service_api.erl#L1015-L1020

[elbrujohalcon]

@okeuday yeah! That makes a lot of sense, actually. I wrote #184 to capture just one of those things.

[elbrujohalcon]

If you think it possible, I'm willing to help with this one.

Would you consider working on this as part of #36, instead? @roberto-aloi and @jfacorro will certainly be willing to help you ;)

To be clear: in my mind, using |-first notation for types is akin to using comma-first notation for lists and other things. That's why I think it would be much more consistent to put all those things in a single rok_formatter instead of cramming them up as options all along default_formatter's code.

On the other hand, if you feel strongly about adding just this change as an option for the default_formatter and sending a PR with that… By all means, give it a try. If the change turns out to be not as massive as I anticipate, it might be a welcomed addition in the end… And make comma-first fans sad because of the delay in the implementation of our favorite formatter :trollface:

[paulo-ferraz-oliveira]

Another data point.

    when StatsInfoContainer =:= memory orelse
         StatsInfoContainer =:= system_info orelse
         StatsInfoContainer =:= statistics ->

gets formatted as

    when StatsInfoContainer =:= memory orelse
             StatsInfoContainer =:= system_info orelse StatsInfoContainer =:= statistics ->

[elbrujohalcon]

In this case, I understand that you prefer for concatenations of orelse's (and probably other infix operators) to be treated as separators in lists (i.e. either all in the same line or each "item" in its own line). Is that correct?

[elbrujohalcon]

If that's the case, I can already tell you that this will not be an easy scenario to handle since the AST doesn't treat the whole "group" of applied infix operations as a single structure; in the AST these things are nested orelse (or other infix operations) applications. So, the formatter analyzes each one individually in a recursive fashion and not as a list of things.

[paulo-ferraz-oliveira]

I understand that you prefer for concatenations of orelse's (and probably other infix operators) to be treated as separators in lists

You understand correctly.

I can already tell you that this will not be an easy scenario to handle

😢

[elbrujohalcon]

We made some progress recently… The original code is now formatted as…

-type x() ::
    {a, very, long, thing} |
    {another, very, long, thing} |
    [yet | another | very | long | thing].

-spec x() ->
           {a, very, long, thing} |
           {another, very, long, thing} |
           [yet | another | very | long | thing].
x() ->
    {a, very, long, thing}
    orelse {another, very, long, thing}
    orelse [yet, another, very, long, thing].