Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Formatter should allow context based indent #57

Open
horkhe opened this Issue Aug 30, 2012 · 18 comments

Comments

Projects
None yet
5 participants
Contributor

horkhe commented Aug 30, 2012

Consider the following code snipet:

NewList = case lists:keyfind(Key, 1, List) of
              false ->
                  lists:umerge(fun({_Key1, Val1}, {_Key2, Val2}) ->
                                       compare(Val1, Val2)
                               end,
                               KeyValList,
                               [{k1, v1},
                                {k2, v2},
                                {k3, v3}])
          end,

Note how:

  1. the body of case indented;
  2. the arguments of lists:umerge indented;
  3. the elements of the explicitly defined list ([{k1, v1}...]) indented.
Contributor

horkhe commented Aug 30, 2012

Another example:

%% gen_server callbacks
-export([init/1,
         handle_call/3,
         handle_cast/2,
         handle_info/2,
         terminate/2,
         code_change/3]).

and one more:

-spec start_link(List) -> {ok, Pid} |
                          {error, Error} when
      List :: list(),
      Pid :: pid(),
      Error :: term().
Contributor

vladimirk commented Aug 30, 2012

This one is emacs style. As for me should be provided as emacs-style formatting profile.

Contributor

horkhe commented Aug 30, 2012

Yes, it is. And totally agree about the emacs-style formatting profile.

Owner

ignatov commented Aug 30, 2012

Oh, I see that Emacs-complatible formatter is really important thing for you, guys.
But before I can implement it, I need more information about how Emacs formatter works.
Some declarative rules will be very helpful.

Another way: provide more settings for the current formatter and create a Emacs style.

Contributor

vladimirk commented Aug 30, 2012

I vote for the latter. Personally I don't like emacs style because of its irregularity indent-wise, for instance this code is from rabbit:

reopen(ClosedHdls) -> reopen(ClosedHdls, get_age_tree(), []).                                                                                                           

reopen([], Tree, RefHdls) ->                                                                                                                                            
    put_age_tree(Tree),                                                                                                                                                 
    {ok, lists:reverse(RefHdls)};                                                                                                                                       
reopen([{Ref, NewOrReopen, Handle = #handle { hdl          = closed,                                                                                                    
                                              path         = Path,                                                                                                      
                                              mode         = Mode,                                                                                                      
                                              offset       = Offset,                                                                                                    
                                              last_used_at = undefined }} |                                                                                             
        RefNewOrReopenHdls] = ToOpen, Tree, RefHdls) ->                                                                                                                 
    case prim_file:open(Path, case NewOrReopen of                                                                                                                       
                                  new    -> Mode;                                                                                                                       
                                  reopen -> [read | Mode]                                                                                                               
                              end) of                                                                                                                                   
        {ok, Hdl} ->                                                                                                                                                    
            Now = now(),                                                                                                                                                
            {{ok, _Offset}, Handle1} =                                                                                                                                  
                maybe_seek(Offset, Handle #handle { hdl          = Hdl,                                                                                                 
                                                    offset       = 0,                                                                                                   
                                                    last_used_at = Now }),                                                                                              
            put({Ref, fhc_handle}, Handle1),                                                                                                                            
            reopen(RefNewOrReopenHdls, gb_trees:insert(Now, Ref, Tree),                                                                                                 
                   [{Ref, Handle1} | RefHdls]);                                                                                                                         
        Error ->                                                                                                                                                        
            %% NB: none of the handles in ToOpen are in the age tree                                                                                                    
            Oldest = oldest(Tree, fun () -> undefined end),                                                                                                             
            [gen_server2:cast(?SERVER, {close, self(), Oldest}) || _ <- ToOpen],                                                                                        
            put_age_tree(Tree),                                                                                                                                         
            Error                                                                                                                                                       
    end.   

I think with this style it's hard to find the lines with the code like this, but it's just my subjectivity talking. But a lot of people used to it so possibility to configure one would be good for them I believe. Another point that there are vim users those might have their own POV how it should look like, so I believe best way to go is make formatting configurable.

Personally I completely satisfied with current formatting style, good job, Sergey!;)

Owner

ignatov commented Aug 30, 2012

What parameters are needed specifically for Emacs style formatting? One big button "Format as Emacs"? Or something else?

BTW, for me code snippet from rabbit looks very messy.

Contributor

horkhe commented Aug 31, 2012

@vladimirk And that is the reason I like it. Tastes differ :)

@ignatov It is important when you work in a team where people use mostly Emacs.

It is probably better if the Erlang Formatter provides enough nobs so that it can be tweaked to produce Emacs like code. In that case Emacs style formatter profile can even be shipped along with the plugin.

Contributor

horkhe commented Aug 31, 2012

@ignatov By the way, even though I have to format code manually, I switched to Intellij from Emacs completely, for it provides significantly better user experience. And for that I can't praise you enough :). Keep up the grate work you do.

@ignatov ignatov added a commit that referenced this issue Sep 10, 2012

@ignatov ignatov alignment initial, part of #10, #57 0db0de4
Owner

ignatov commented Sep 16, 2012

@horkhe, @vladimirk please try the latest build. I've just added alignment for function clauses:

test([])            -> ok;
test(AlignmentTest) -> ok.
Contributor

vladimirk commented Sep 16, 2012

Oh no,no,no! Even emacs doesn't work that way. One of the things about emacs formatter that it has shortcuts to align that way but it does not reformat all sources that way!
One of the things I don't like about emacs code rules that those are not universal - you can have it couple of ways, all are accepted but none of them applicable to all cases.
It you wanna go support emacs formatting it should be extremely optional almost for every rule.

This is emacs formatting style: https://github.com/erlang/otp/blob/maint/lib/tools/emacs/test.erl.indented
As you can see in there a lot of ways of indenting same thing and with emacs formatter they are all coexist in same source.

All these rules should be optional. Previous version of formatter was perfect.

Contributor

horkhe commented Sep 17, 2012

@ignatov I totaly agree with @vladimirk everything should be adjustable. As for this particular formatting element, even I emacs-style lover do not use it since it is not a default.

Owner

ignatov commented Sep 17, 2012

Sure, I'll add the option for such indentation (false by default).

I know that is not native solution but maybe we could use it same way (as option)

https://gist.github.com/3954316

Owner

ignatov commented Oct 25, 2012

Hi, @andrzejsliwa, thanks for the useful advice. At the present @yzh44yzh tries to implement such approach. Please, see #10 (comment).

@ignatov yes, but my version is not depends from local .emacs.d configuration and run in batch mode

Owner

ignatov commented Oct 26, 2012

OK, thanks again.

@andrzejsliwa https://gist.github.com/3954316 ok, this code is a little bit better. I will take it into account )

Owner

ignatov commented May 24, 2013

Something for case expression indentation I've pushed in c806c76.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment