Lombiq / NodeJs-Extensions

Utilities and extensions for Node.js, used in ASP.NET (Core) MVC and Orchard (Core) CMS development.
BSD 3-Clause "New" or "Revised" License
7 stars 2 forks source link

Markdownlint reporting false positive "double-spaces" (OSOE-831) #97

Open BenedekFarkas opened 3 months ago

BenedekFarkas commented 3 months ago

In OSOCE, but only in the Windows build, after merging the https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/pull/700 PR, which updated Lombiq.GitHub.Actions with changes from the https://github.com/Lombiq/GitHub-Actions/pull/325 PR. See the build log here, here are relevant entries:

Error: doubled-spaces: Found doubled spaces. [D:\a\Open-Source-Orchard-Core-Extensions\Open-Source-Orchard-Core-Extensions\src\Utilities\Lombiq.NodeJs.Extensions\Lombiq.NodeJs.Extensions.SolutionMarkdownAnalysis\Lombiq.NodeJs.Extensions.SolutionMarkdownAnalysis.csproj]
Error: doubled-spaces: Found doubled spaces. [D:\a\Open-Source-Orchard-Core-Extensions\Open-Source-Orchard-Core-Extensions\src\Utilities\Lombiq.NodeJs.Extensions\Lombiq.NodeJs.Extensions.SolutionMarkdownAnalysis\Lombiq.NodeJs.Extensions.SolutionMarkdownAnalysis.csproj]

This doesn't help identifying where the problem is, but a local VS build points to LGHA's root Readme.md file, on lines 41 and 42:

To release versions of Lombiq GitHub Actions, and allow consumers to reference a specific version of a reusable workflow or composite action (e.g. `@v1.0`), we employ some automation to do this in a consistent and predictable way.
See [issue #284 "Introduce versioning and releases (OSOE-735)"](https://github.com/Lombiq/GitHub-Actions/issues/284) <!-- #spell-check-ignore-line -->

These lines contain no double spaces.

Jira issue

Piedone commented 3 months ago

So it's not that the error is a false positive, but that the error message is incomplete/misleading, right? Because when you see

Error: doubled-spaces: Found doubled spaces. [D:\a\Open-Source-Orchard-Core-Extensions\Open-Source-Orchard-Core-Extensions\src\Utilities\Lombiq.NodeJs.Extensions\Lombiq.NodeJs.Extensions.SolutionMarkdownAnalysis\Lombiq.NodeJs.Extensions.SolutionMarkdownAnalysis.csproj]

It just means that NE's MD analysis has sent this error, which is expected, and always the case (since that project runs that linter Node command). It doesn't mean that the error is in NE. There should be an additional line of output somewhere around this (and this used to work, or sometimes works) that tells you the actual file.

BenedekFarkas commented 3 months ago

Like I wrote above, the error (warning) is a false positive and Lombiq.NodeJsExtensions houses and runs markdownlint, so it's this project's responsibility.

BenedekFarkas commented 3 months ago

Correction, it's textlint-rule-doubled-spaces. According to its NPM page, it uses the /\s{2,}/g regex, which doesn't actually match for the lines reported as problematic (regardless of regex flavor).

This change mitigated the warnings.

Piedone commented 3 months ago

I'm not arguing about responsibility, but the nature of the issue.

I guess BTW why this rule tripped is because of two adjacent lines (you should rather have a single line, how you fixed it, or lines separated by blank lines, making the paragraphs).

BenedekFarkas commented 3 months ago

That's a workaround, yes, but that package is designed to detect double spaces (which it does), but it also detects a specific way of formatting text, which is completely normal and valid Markdown syntax, while it has nothing to do with actual double spaces.

Piedone commented 3 months ago

So two things to do here:

BenedekFarkas commented 3 months ago

Updating textlint won't have an effect, because textlint-rule-doubled-spaces is a separate package and we're already using its latest version from February 2023.

The first issue is rather finding out why this package behaves differently on Windows vs. Linux and try to contrib a fix or just create our own rule for it.

Piedone commented 1 month ago

When running an OSOCE rebuild (not simply build) locally from VS, you'll see the warning in the Error List too, with the file and line (and clicking it will bring you to it). Running the task directly shows the location of the violation too:

image

BTW this is a false positive as well:

> [!NOTE]
> The code samples in the documentation reference the latest versions of the workflows and actions from the...

Adding a line break between the lines fixes it but there shouldn't be a line break (it breaks this formatting).

Instead, disabling textlint for that block would be a proper workaround, see below, for which textlint-filter-rule-comments is needed:

<!-- textlint-disable -->

> [!NOTE]
> The code samples in the documentation reference the latest versions of the workflows and actions from the...

<!-- textlint-enable -->

Note the newlines around the disable block! https://github.com/textlint/textlint-filter-rule-comments#:~:text=Limitation(markdown)%3A

However, I couldn't get this to work. This is supposed to work: https://github.com/Lombiq/NodeJs-Extensions/commit/cf64bf0766d831df963586bf4f70df48b6a6d91c But adding the disable comment like above doesn't have an effect. I asked about this: https://github.com/textlint/textlint/discussions/1389.

Piedone commented 1 month ago

I fixed disable comments in https://github.com/Lombiq/NodeJs-Extensions/pull/101. This issue is still applicable though.

Piedone commented 3 days ago

https://github.com/Lombiq/GitHub-Actions/issues/365 might be the cause for not seeing the file and line reference.