Skip to content

Inconsistent indentation checking #297

New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Bartheleway opened this issue Oct 14, 2024 · 9 comments · May be fixed by #298
Open

Inconsistent indentation checking #297

Bartheleway opened this issue Oct 14, 2024 · 9 comments · May be fixed by #298

Comments

@Bartheleway
Copy link
Contributor

Issue

My team and I started using this very good package in our CI.
We found out that multiline comments in javascript files (probably all languages recognizing this kind of comments /** */) are wrongly spotted as "INDENTATION_TABS" because we are using indent_style = tab (editorconfig). Same applies for indent_style = space with the error "INDENTATION_SPACES_AMOUNT"

Example code

class MyClass {
    /**
     * @ description Do something
     */
    function myFunction () {
        ...
    }
}

Tests

I made my test using lintspaces-cli which might be the source of issue as well.

.editorconfig value expected behavior result comment
indent_style = space ok ok? Please confirm this test, lintspaces-cli seems to set it to 0 instead of using the default 4
indent_style = space & indent_size = 4 ok ko Fails on the line with * @description ...
indent_style = tab ok ko Fails on the line with * @description ...

Possible solution

I see two solutions for this:

  • Simple: we make an exception when we find <space>*<space>, this could create false negative (not detecting wrong indentation)
  • Complex: we make an exception when we find a multiline comment, this oblige to find when it starts and when it ends

I feel this is a language specific behavior so complex solution will probably be more robust approach.

@Bartheleway Bartheleway linked a pull request Oct 27, 2024 that will close this issue
@schorfES
Copy link
Owner

Hello @Bartheleway, thanks for your feedback. Did you tried to use the ignores option to handle this? This is mainly an issue with the style of such doc string comments. So if you use the option ignore with the build in ignores: ['js-comments'] this should ideally solve your issue.

@Bartheleway
Copy link
Contributor Author

Bartheleway commented Nov 6, 2024

@schorfES, I wasn't aware of this option. I tried it today and result is pretty good.

I see at least one issue of using it. It seems to ignore all checks between comment boundaries which is not what I am trying to achieve.

For example (with explicit indent / EOL):

/**<lf>
<space><space>some bad stuff * could be bad line ending<cr><lf>
<tab><space><space>whatever
*/<lf>

This would pass regardless of line ending and indentation settings. In my use case I only want to accept the extra whitespace in front of the * on multiline comments.

/**<lf>
<space>* Some comments<lf>
<space>*/<lf>

I see that my PR is only handling js comment style so it needs to be improved. The good thing is it only add acceptance for that extra front space before the * without disabling other checks.

I know that it is only my use case (following VS Code / js doc / java doc / ... convention) and that other projects might want to fully ignore what is between comment boundaries so I think both system should coexist. Happy to discuss it further and to improve my PR accordingly 😄

@Bartheleway
Copy link
Contributor Author

I was confused around the trailingspacesToIgnores option, naming can probably be improved. Now I understand that this allow to choose activation in ignores. Maybe we can create eolToIgnores et indentationToIgnores on the same model.

@schorfES
Copy link
Owner

schorfES commented Nov 7, 2024

I’m a bit confused as well, but as far as I remember, EOL is always validated when the option is turned on, regardless of any ignores affecting the line (see README). Indentation, however, is disabled by ignores specifically for cases where comments have “incorrect” indentation. So, I believe the new options might not be necessary. I’ll need to double-check this...

@schorfES
Copy link
Owner

@Bartheleway I checked all the options. Here’s a list showing which ones respect ignores 👍 and which do not 👎:

As you can see, most of the options respect ignores, except for these two options. The end of line option does not consider ignored lines and always validates line endings, as it’s generally undesirable to mix line endings, whether in code or comments. I think this aligns with what you expect. So from my current perspective, I don’t see a need for the two new settings, eolToIgnores and indentationToIgnores. Hope I didn’t misunderstand you.

@Bartheleway
Copy link
Contributor Author

Bartheleway commented Nov 18, 2024

Yes I agree, I don't really understand how I got no report about EOL inside comment as reading the code, it should raise it ...

The only remaining "issue" is that indentation option respect too much the ignore option to my taste 😄 I would expect to still validate the indentation on comment lines in front of * instead of just not validating the line.

Maybe this is just my edge case and I am the only one concerned about comments like:

class MyClass {
    /**
Weird stuff
 */
    function myMethodWithBadComment () {
        // Some code
    }

    /**
     * Weird stuff
     */
    function myMethodWithGoodComment () {
        // Some code
    }
}

@schorfES
Copy link
Owner

Hi @Bartheleway, thank you for double-checking. If you still encounter a case where EOL is not working as expected, please consider opening a PR with a test case so we can address it.

Regarding indentation for ignored lines, this primarily relates to the scope of this project. To fulfill your request, Lintspaces would need to become a syntax parser utilizing an AST, similar to ESLint and similar tools. However, Lintspaces is designed to be language-agnostic, focusing on rules that apply to any text (or source code) file rather than specific language syntax.

While the multiline comment style with a vertical line of * characters (/** ... */) originates from Javadoc in Java and has been adopted by languages like JavaScript for documentation tools, your request involves more than simple text line checking—it introduces a dependency on specific language syntax. Although this project is written in JavaScript and is likely used primarily in JavaScript or web-based projects, Lintspaces aims to remain flexible and usable with any kind of text file.

For this reason, I believe adding built-in support for this type of comment exceeds the scope of the project. However, we can explore solutions that maintain the project’s language-agnostic nature. One idea is to introduce hooks to Lintspaces, which would allow users to extend the linting process with custom code for extra validations or modifications to the report. In this case, a hook could be used to add a regular expression to check whether the * is correctly indented for ignored lines.

What do you think of this approach?

@Bartheleway
Copy link
Contributor Author

Interesting approach. Going this way would probably means that we will see in a near future some "node-lintspaces-js", "node-lintspaces-java", ...

  • each package will almost have no code except the customisation of the indentation behavior for comments
  • every new node-lintspaces version would involve new versions of these packages
  • linting a multiple language repo will involve several packages of node-lintspaces-*
    In case no child package appears, this will means people will copy/paste the piece of code that goes into the hook on several projects. I feel having a pre-built option rather than code hook option is more neat.

Another approach would be to get all these language specific constraints in the current package. It will certainly alter a bit the agnostic current nature of the package which for me is fine as long as we don't break it (only linting specific language). As you said, vertical aligned comments comes from java and are now used in other languages. This can be seen as this type of comment is now language agnostic it is just a style of comment which can be then supported by a language agnostic linter 😉 in the same matter the package already implement language specific ignore pattern and this doesn't break the agnostic nature of the package.

Technically speaking, I don't know yet how to implement it correctly but I will definitely explore this path.

@schorfES
Copy link
Owner

I would not expect separate packages for different languages, as the style of these comments is the same across languages. Technically, "node-lintspaces-js" and "node-lintspaces-java" would have nearly identical code. Instead, I can think of shipping these hooks or plugins directly with lintspaces and importing them as needed. For example:

import Validator from 'lintspaces';
import DocStringHook from 'lintspaces/plugins/docstring';

const validator = new Validator({
  hooks: [DocStringHook],
});
validator.validate('/path/to/file.ext');
validator.validate('/path/to/other/file.ext');

const results = validator.getInvalidFiles();

I assume, this approach would address your point as well:

This can be seen as this type of comment is now language agnostic it is just a style of comment which can be then supported by a language agnostic linter 😉 in the same matter the package already implement language specific ignore pattern and this doesn't break the agnostic nature of the package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants