JCF Incorrect indenting for try/finally

[award65]

I've been experimenting with Jedi Code Formatter, trying to find the options that suit me. My personal preference is what in the C world is called "Whitesmiths" style - begin/end pairs are indented at the same level as the block they contain, which to me (religious war coming...) makes sense as it matches the Pascal language definition.

Anyway, this can be achieved with JCF using the "Extra indent for being/end inside procedures option", with this result:


procedure SaveYamlObject(AEmitter: TYamlEmitter; const AName: string;
  AValue: TOTPersistent);
begin
  AEmitter.ScalarEvent('', '', AName, True, False, yssPlainScalar);
  if Assigned(AValue) then
    begin
    AValue.SaveToYaml(AEmitter);
    end
  else
    begin
    AEmitter.MappingStartEvent('', '', False, ympFlowMapping);
    AEmitter.MappingEndEvent;
    end;
end;


The downside is that it also messes up try/finally/except blocks, producing this....


    emitter := TYamlEmitter.Create;
      try
      stream := TStringStream.Create;
      emitter.SetOutput(stream);
      yamlVer.Initialize;

      emitter.StreamStartEvent;
      emitter.DocumentStartEvent(yamlVer, nil, True);

      owning.SaveToYaml(emitter);

      emitter.DocumentEndEvent(True);
      emitter.StreamEndEvent;

      yamlText := stream.DataString;
      finally
      emitter.Free;
      end;


which should be formatted as:


    emitter := TYamlEmitter.Create;
    try
      stream := TStringStream.Create;
      emitter.SetOutput(stream);
      yamlVer.Initialize;

      emitter.StreamStartEvent;
      emitter.DocumentStartEvent(yamlVer, nil, True);

      owning.SaveToYaml(emitter);

      emitter.DocumentEndEvent(True);
      emitter.StreamEndEvent;

      yamlText := stream.DataString;
    finally
      emitter.Free;
      end;


(ie the try/finally should NOT be indented).

The cuplrit is in Indenter.pas:


  // IndentBeginEnd option to indent begin/end words a bit extra
  if FormattingSettings.Indent.IndentBeginEnd then
  begin
    if (pt.TokenType in [ttTry, ttExcept, ttFinally, ttBegin, ttEnd]) and InStatements(pt) then
    begin
      // filter out the begin/end that starts and ends a procedure
      if not pt.HasParentNode(nBlock, 2) then
      begin
        Result := Result + FormattingSettings.Indent.IndentBeginEndSpaces;
      end;
    end;
  end;


Is there any good reason why ttTry, ttExcept and ttFinally are being indented here?

This looks like a simple fix for me to make, but if I want a PR to be accepted for this should there be a new option added so that formatting for any existing users of this option won't be affected?

Alistair.

[TRon]

...which to me (religious war coming...) makes sense as it matches the Pascal language definition.

No flames from me (everyone has his/her own preferred style, and as long as I don't have to work with it  ) but it would have made a lot more sense if you would have used code-tags

[rvk]

Code: Pascal  [Select][+][-]

1. procedure SaveYamlObject(AEmitter: TYamlEmitter; const AName: string;
2.   AValue: TOTPersistent);
3. begin
4.   AEmitter.ScalarEvent('', '', AName, True, False, yssPlainScalar);
5.  

I'm just curious. If you say it matches pascal definition to indent begin, why did you not indent the begin below the procedure (highlighted line)?

No, I'm not a fan of indenting begin itself.

[Lansdowne]

I do not use any auto formatters, but I  don't think it's reasonable to call this option "Incorrect".

Comparing your if...then...else example with the try..finally example,

 'try' performs the dual role of  'if x then' and 'begin'.
'finally' performs the role of 'end' then 'else' then 'begin'.
Then you have 'end' in both examples.

The option gives priority to 'try' playing the role of 'begin', whereas you would wish it to give priority to its role as introducing the block, equivalent to 'if...then'.

Personally I would see your preferred layout as "incorrect" because the final 'end;' is at an indent level that does not show a 'begin' at the same level N lines above.  So given your wish to pull out the 'try' and 'finally' lines, I would expect to pull out the 'end;' as well.

And no, I wouldn't use code tags here, because I wouldn't want the code tag to change the way it presents the code under discussion.

[DomingoGP]

Is there any good reason why ttTry, ttExcept and ttFinally are being indented here?

consider this code, the try and finally should have the same identation as the end in my opinion.

Code: Pascal  [Select][+][-]

1.  
2.  if condition then
3.     try
4.     d:=1;
5.     finally
6.     d:=2;
7.     end;
8.  

This looks like a simple fix for me to make, but if I want a PR to be accepted for this should there be a new option added so that formatting for any existing users of this option won't be affected?

Yes, at least one new option has to be introduced to maintain compatibility with the current code and I am not yet sure if more are needed to cover all corner cases.


It's not a simple fix, there are many options to format the code, In your opinion, is this code format correct?
I believe this can be achieved with just one new option.

Code: Pascal  [Select][+][-]

1.  
2. procedure rr;
3. begin
4.   if condition then
5.     begin
6.     a := -1;
7.     try
8.       a := 0;
9.     finally
10.       a := 1;
11.     end;
12.     end;
13.  
14.   if condition then
15.   try
16.     a := 0;
17.   finally
18.     a := 1;
19.   end;
20. end;  
21.  

[award65]

Firstly, my apologies for not using code tags :-(

I would format the example block as:

Code: Pascal  [Select][+][-]

1. procedure rr;
2. begin
3.   if condition then
4.     begin
5.     a := -1;
6.     try
7.       a := 0;
8.     finally
9.       a := 1;
10.       end;
11.     end;
12.  
13.   if condition then
14.     try
15.       a := 0;
16.     finally
17.       a := 1;
18.       end;
19.  

The difference is I would indent the final "end" of the try/finally/except to align with the code in the finally/except clause.

Many years ago when I was writing the internal Delphi coding standard at a previous job, we wrestled with how this should be formatted. Our conclusion at the time was that the implementers of try/finally/except f***ed up and chose an "un-Pascal" syntax by having a trailing "end" with no matching begin. We stuck with begin/end pairs always being indented to match the code they enclosed, and in the few cases where the language has an un-paired "end", we indented that to align with the code as well.

And I would never write code like the second "if" - anything more complex than a simple statement would always be inside a begin/end pair.

I'm happy to introduce another option to prevent the indenting of try/finally/except when the "Extra indent for begin/end inside procedures" is selected, but I would still contend that this is a bug in existing behaviour, as the option as listed in the UI says nothing about indenting try/finally/except in the first place...

[DomingoGP]

I have the changes to support this style ready, I needed 4 new options to add to the <Indent> section of the configuration file.

<IndentExtraTryBlockKeyWords> False </IndentExtraTryBlockKeyWords>
<IndentExtraTryBlockKeyKeyWordsSpaces> 2 </IndentExtraTryBlockKeyWordsSpaces>
<IndentEndTryBlockAsCode> False </IndentEndTryBlockAsCode>
<IndentExtraOrphanTryBlocks> False </IndentExtraOrphanTryBlocks>

Before committing I want to test it more and include the modifications in the lazarus GUI, but if you want to test it I include the patch here.

Regards
Domingo

[BrunoK]

How / where does one parametrize JCF to get the following code :

Code: Pascal  [Select][+][-]

1. procedure rr;
2. begin
3.   if condition then
4.   try
5.     a := 0;
6.   finally
7.     a := 1;
8.   end;
9. end;

to be reformatted to :

Code: Pascal  [Select][+][-]

1. procedure rr;
2. begin
3.   if condition then
4.     try
5.       a := 0;
6.     finally
7.       a := 1;
8.     end;
9. end;

It seems that it insists aligning, when there is no begin, the try below the if.
The column of end of the try .. finally ... end; should be logically at the same position as the try, like is the case for the case structure block.

[DomingoGP]

How / where does one parametrize JCF to get the following

with the patch attached applied, you must set

Code: [Select]

<IndentExtraOrphanTryBlocks> True </IndentExtraOrphanTryBlocks>in the JCFsettings.cnf

The column of end of the try .. finally ... end; should be logically at the same position as the try, like is the case for the case structure block.

I also thing the same, but the OP preffers the end aligned with the code. The option <IndentEndTryBlockAsCode> controls this.

Code: [Select]

<IndentEndTryBlockAsCode> False </IndentEndTryBlockAsCode>

[Thaddy]

to be reformatted to :

Code: Pascal  [Select][+][-]

1. procedure rr;
2. begin// block start 1
3.   if condition then//block start 2
4.     try//block start 3
5.       a := 0;
6.     finally // block 3
7.       a := 1;
8.     end;//end block 3
9.    //implied block 2 condition resolving not existing else, if block has no else.
10. end;// block start 1

That is actually preferred by many.
My comments explain how it should be done.

[BrunoK]

Now starting with :

Code: Pascal  [Select][+][-]

1. procedure rr;
2. begin
3.   if condition then
4.   try
5.   a := 0;
6.   finally
7.   a := 1;
8.   end;
9. end;

Ctrl+d ends up with :

Code: Pascal  [Select][+][-]

1. procedure rr;
2. begin
3.   if condition then
4.       try
5.       a := 0;
6.       finally
7.       a := 1;
8.       end;
9. end;

What else should I change ?

[DomingoGP]

I guess you need.

Code: [Select]

<IndentExtraTryBlockKeyWords> False </IndentExtraTryBlockKeyWords>
<IndentExtraTryBlockKeyWordsSpaces> 2 </IndentExtraTryBlockKeyWordsSpaces>

if this not work please attach your JCFSettings.cfg

[BrunoK]

if this not work please attach your JCFSettings.cfg

[DomingoGP]

I modifed lines 27 and 43 and works fine for me.

[BrunoK]

I modifed lines 27 and 43 and works fine for me.

That's it.

Thank you very much.

Bruno.