#442 line breaks before the comma for uses lists can comment out the unit names

[Thomas Mueller]

Enabling line breaks before the comma for uses lists will comment out the unit names if the line before them had a comment. This ...

uses
  Windows, // comment1
  SysUtils, // comment2
  Classes, // comment3
  Graphics, // comment4
  Forms; // comment5

... gets formatted as this ...

uses
  Windows
  , // comment1SysUtils
  , // comment2Classes
  , // comment3Graphics
  , // comment4Forms
  ; // comment5

The culprit is the combination of the two Line Breaks options

• Between every unit in uses
  • before the comma

[Christoph "baka0815" Schwerdtfeger]

I think the preferred way to format

uses
  Windows, // comment1
  SysUtils, // comment2
  Classes, // comment3
  Graphics, // comment4
  Forms; // comment5

would be

uses
  Windows // comment1
, SysUtils // comment2
, Classes // comment3
, Graphics // comment4
, Forms // comment5
;

I will take a look at this.

[Christoph "baka0815" Schwerdtfeger]

Here is a patch for the formatter.

[Christoph "baka0815" Schwerdtfeger]

And here the patch for the unit tests.

[Thomas Mueller]

The patch seems to fix the bug. But the output is not what you proposed above. It looks like this:

uses
  Windows // comment1
  , SysUtils // comment2
  , Classes // comment3
  , Graphics // comment4
  , Forms // comment5
  ;

but you suggested this:

uses
  Windows // comment1
, SysUtils // comment2
, Classes // comment3
, Graphics // comment4
, Forms // comment5
;

The difference is the indentation of the comma.

Since I don't use that kind of formatting, I don't really care that much either way. So if you are happy with it, I'll accept it.

Regarding the code I saw that you introduced a rather generically named variable which I don't like. But I will need to have a closer look.

Edit: What about moving the whole code block handling the rtComma case to a function that returns True if CheckIndent should be exited? That would allow to remove the additional variable and make the code a bit more readable. (I don't like that huge case statement anyway, but never could be bothered to break it up.)

[Christoph "baka0815" Schwerdtfeger]

The difference is the indentation of the comma.

That depends on the settings (see TestDoNotIndentUsesStartingWithComma1Comment vs. TestIndentUsesComment:

      ExpectedText.Add('uses');
      ExpectedText.Add('  Windows // Comment 1');
      ExpectedText.Add('  , Graphics // Comment 2');
      ExpectedText.Add('  , Forms // Comment 3');
      ExpectedText.Add('  ;');

vs.

      ExpectedText.Add('uses');
      ExpectedText.Add('  Windows // Comment 1');
      ExpectedText.Add(', Graphics // Comment 2');
      ExpectedText.Add(', Forms // Comment 3');
      ExpectedText.Add(';');
      ExpectedText.Add('');

Edit: What about moving the whole code block handling the rtComma case to a function that returns True if CheckIndent should be exited? That would allow to remove the additional variable and make the code a bit more readable. (I don't like that huge case statement anyway, but never could be bothered to break it up.)

That is something I will look at.

[Christoph "baka0815" Schwerdtfeger]

Edit: What about moving the whole code block handling the rtComma case to a function that returns True if CheckIndent should be exited? That would allow to remove the additional variable and make the code a bit more readable. (I don't like that huge case statement anyway, but never could be bothered to break it up.)

That is something I will look at.

Would it be ok, if I change that in the other patch-set ( https://sourceforge.net/p/gexperts/feature-requests/198/ )?

[Thomas Mueller]

Yes, do that. (Sorry for taking so long to answer, I haven't got much time currently.) This will hopefully get better next week.

[Christoph "baka0815" Schwerdtfeger]

The rather generically named variable would be IdxChanged, right?
This could be named TokenIndexChanged or similar, if you prefer.

Are there other things missing or in need of change?

[Thomas Mueller]

Yes, IdxChanged was the variable I was referring to. But if you move the code to a separate method, you don't need to change the variable name because it will be local to the new method.
The rest should be fine.

[Thomas Mueller]

patch applied in revision #4864