Allow comments in selective imports.

[veelo]

tokenLength() does not consider comments as valid tokens, so get their length explicitly.

Fixes issue 384, allowing comments inbetween the symbols of selective imports.

[Hackerpilot]

I don't remember why tokenLength doesn't have a case for handling comments. Does adding support to that function break anything any of the unit tests?

[veelo]

I don't remember why tokenLength doesn't have a case for handling comments. Does adding support to that function break any of the unit tests?

Haven't tried that. I didn't dare changing tokenLength as I don't really understand the purpose of that function. In fact, on second thought, I think using tokens[i].text.length instead of tokenLength irrespective of tokens[i].type should work, as the only acceptable tokens in that spot other than separators and whitespace are identifiers and comments. Would you agree?

[veelo]

I don't remember why tokenLength doesn't have a case for handling comments. Does adding support to that function break any of the unit tests?

Haven't tried that.

I can't easily run the tests because I am on Windows a.t.m. and test.sh is a shell script. I actually considered porting it to rdmd but didn't.

[ghost]

I was about to push a test to your fork but actually the fix doesn't work. It addresses the assertion failure but formating is not good:

import std.algorithm, std.stdio : readln,
                                  // comment
                                  writeln;

gives

import std.algorithm, std.stdio : readln, // comment
writeln;

[ghost]

With this patch (applied to the PR in its current state) the formatting is correct although there must be better ways

@@ -525,10 +525,21 @@ private:
                 writeToken();
                 write(" ");
             }
             else if (currentIs(tok!","))
             {
+                if (index + 1 < tokens.length && tokens[index + 1].type == tok!"comment")
+                {
+                    writeToken();
+                    write(" ");
+                    writeToken();
+                    simpleNewline();
+                    if (!indentLevel)
+                        indentLevel++;
+                    indent();
+                    continue;
+                }
                 // compute length until next ',' or ';'
                 int lengthOfNextChunk;
                 for (size_t i = index + 1; i < tokens.length; i++)
                 {
                     if (tokens[i].type == tok!"," || tokens[i].type == tok!";")

[Hackerpilot]

although there must be better ways

This is how I feel about almost all the code in this project.

[Hackerpilot]

@bbasile what does your proposed change do when the comment following the comma is a /* */ comment instead of //?

[ghost]

Nothing and maybe indentation is wrongly handled too (should it be always incremented ?)

[ghost]

Anyway, @veelo , you can use the online tests and amend, force push, until it becomes green.

[veelo]

To be frank, I have noticed the formatting issues as well, more on that below. In my eyes however, a formatting bug is a lot less severe than aborting the program and discarding most of the source (especially when used --inplace) which is a blocking bug. I'd like to get this blocking bug fixed asap and open a new issue on the formatting.

About formatting: in my quest to resolve the formatting I quickly digressed into other parts of the code like newline(), indenting, the handling of comments, etc. which for me, being unfamiliar with the code, is uncomfortable to work with. It is difficult to see why things are coded as they are.

I have worked with this test file:

import std.stdio : readln, writeln;
import std.stdio : readln,
                   writeln;
import std.stdio : readln,

                   writeln;
import std.stdio : readln, /* comment1 */ writeln;
import std.stdio : readln, // comment2
                   writeln;
import std.stdio : readln,
                   // comment3
                   writeln;
import std.stdio : readln,
                   /* comment4 */
                   writeln;
/*
123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
         10        20        30        40        50        60        70        80
*/
import std.stdio : readln, readln, readln, readln, readln, readln, readln, readln, readln, readln, readln,

                   writeln;
import std.stdio : readln, readln, readln, readln, readln, /* comment5 -------- */ readln, readln, readln, readln, readln, readln,

                   writeln;
import std.stdio : readln, readln, readln, readln, readln, readln, readln, readln, readln, readln, readln,
                   // comment6
                   writeln;
import std.stdio : readln, readln, readln, readln, readln, readln, // comment7
                   // comment8
                   writeln;

which is formatted, by the PR in its current state, using --max_line_length=70 --soft_max_line_length=70, as

import std.stdio : readln, writeln;
import std.stdio : readln, writeln;
import std.stdio : readln, writeln;
import std.stdio : readln,  /* comment1 */ writeln;
import std.stdio : readln,  // comment2
    writeln;
import std.stdio : readln, // comment3
writeln;
import std.stdio : readln, /* comment4 */
    writeln;

/*
123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
         10        20        30        40        50        60        70        80
*/
import std.stdio : readln, readln, readln, readln, readln, readln,
    readln, readln, readln, readln, readln, writeln;
import std.stdio : readln, readln, readln, readln, readln, /* comment5 -------- */ readln,
    readln, readln, readln, readln, readln, writeln;
import std.stdio : readln, readln, readln, readln, readln, readln,
    readln, readln, readln, readln, readln, // comment6
    writeln;
import std.stdio : readln, readln, readln, readln, readln, readln, // comment7
    // comment8
    writeln;

As you see, the statements with comment5 and comment7 violate the max_line_length, and I don't understand why. I don't understand why the statements with comment3 and comment4 are formatted differently. @bbasile addressed comment3 (thank you) but it seems to skip the test on config.dfmt_soft_max_line_length further down, so I don't think that this change suffices when lines are longer. Note that comment6 and comment8 don't show the defect addressed by @bbasile, which adds to my confusion.

In principle, the formatting of selective imports is much simpler than other code: the indenting is fixed, the types of tokens are very limited (just identifiers and comments and an occasional alias). This should be easy... I'd almost just grab std.string.wrap.

[veelo]

Thought: Can't we just reuse the same code for the formatting of the definition of enumerations, inside { and }? That also is just a comma separated list of identifiers with optional assignments. Does it handle comments inside the list acceptably? (If it doesn't, it probably should).

[ghost]

Yes indeed, enum formating seems to handle comments enough closely to what's expected for imports.

[veelo]

Yes indeed, enum formating seems to handle comments enough closely to what's expected for imports.

But enum members are put on separate lines, which is not how selective imports are formatted currently. I'm on to something though, hang on.

[veelo]

What my last commit does not fix is line length violations where comments are involved:

/*
123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
         10        20        30        40        50        60        70        80
*/
import std.stdio : readln, readln, /* commentA */ readln, readln, readln, readln, readln, readln, readln, readln, readln,
                   writeln;
import std.stdio : readln, readln, readln, readln, readln, /* commentB -------- */ readln, readln, readln, readln, readln, readln,
                   writeln;
import std.stdio : readln, readln, readln, readln, readln, readln, readln, // commentC
                   // commentD
                   writeln;

void fun(int EEEEEEE1, int EEEEEEE2, int EEEEEEE3, int EEEEEEE4, int EEEEEEE5, /* comment */ int EEEEEEE6) {}
void gun(int EEEEEEE1, int EEEEEEE2, int EEEEEEE3, int EEEEEEE4, // commmmmmmmmmmmmmmmmmmmment
      int EEEEEEE5) {}

is formatted as (EDIT: with --max_line_length=80)

/*
123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
         10        20        30        40        50        60        70        80
*/
import std.stdio : readln, readln, /* commentA */ readln, readln, readln, readln, readln,
    readln, readln, readln, readln, writeln;
import std.stdio : readln, readln, readln, readln, readln, /* commentB -------- */ readln, readln,
    readln, readln, readln, readln, writeln;
import std.stdio : readln, readln, readln, readln, readln, readln, readln, // commentC
    // commentD
    writeln;

void fun(int EEEEEEE1, int EEEEEEE2, int EEEEEEE3, int EEEEEEE4, int EEEEEEE5, /* comment */ int EEEEEEE6)
{
}

void gun(int EEEEEEE1, int EEEEEEE2, int EEEEEEE3, int EEEEEEE4, // commmmmmmmmmmmmmmmmmmmment
        int EEEEEEE5)
{
}

Function arguments share this defect so it should be considered another issue, I'd say.

[Hackerpilot]

I'd like to get this blocking bug fixed asap and open a new issue on the formatting.

Agreed and merged.

[veelo]

I'd like to get this blocking bug fixed asap and open a new issue on the formatting.

Agreed and merged.

Thanks. Can we have a new release, please? :-)