Skip to content
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

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

Open
BenedekFarkas opened this issue Mar 21, 2024 · 10 comments
Open

Comments

@BenedekFarkas
Copy link
Member

BenedekFarkas commented Mar 21, 2024

In OSOCE, but only in the Windows build, after merging the Lombiq/Open-Source-Orchard-Core-Extensions#700 PR, which updated Lombiq.GitHub.Actions with changes from the Lombiq/GitHub-Actions#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

@github-actions github-actions bot changed the title Markdownlint reporting false positive "double-spaces" Markdownlint reporting false positive "double-spaces" (OSOE-831) Mar 21, 2024
@Piedone
Copy link
Member

Piedone commented Mar 21, 2024

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
Copy link
Member Author

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
Copy link
Member Author

BenedekFarkas commented Mar 21, 2024

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
Copy link
Member

Piedone commented Mar 21, 2024

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
Copy link
Member Author

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
Copy link
Member

Piedone commented Mar 22, 2024

So two things to do here:

  • Check if the latest textlint (v14.0.4, we're on v13.3.3) has the same issue. If not, then upgrade to it, if yes, report an issue.
  • Fix that such violations are surfaced without the offending file being referenced (this might be an issue for markdownlint too but I think not).

@BenedekFarkas
Copy link
Member Author

BenedekFarkas commented Mar 24, 2024

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
Copy link
Member

Piedone commented Jun 5, 2024

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: cf64bf0 But adding the disable comment like above doesn't have an effect. I asked about this: textlint/textlint#1389.

@Piedone
Copy link
Member

Piedone commented Jun 6, 2024

I fixed disable comments in #101. This issue is still applicable though.

@Piedone
Copy link
Member

Piedone commented Jul 2, 2024

Lombiq/GitHub-Actions#365 might be the cause for not seeing the file and line reference.

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

No branches or pull requests

2 participants