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

lintcmd: don't print \' in -list-checks and -explain #1544

Closed
wants to merge 2 commits into from

Conversation

arp242
Copy link
Contributor

@arp242 arp242 commented May 25, 2024

All the single quotes are written as \' in the descriptions. I assume that's for a good reason, but -list-checks would print:

% staticcheck -list-checks | grep "'"
S1003 Replace call to \'strings.Index\' with \'strings.Contains\'
S1004 Replace call to \'bytes.Compare\' with \'bytes.Equal\'
S1011 Use a single \'append\' to concatenate two slices
S1012 Replace \'time.Now().Sub(x)\' with \'time.Since(x)\'
...

% staticcheck -explain S1003
Replace call to \'strings.Index\' with \'strings.Contains\'

So fix that:

% staticcheck -list-checks | grep "'"
S1003 Replace call to 'strings.Index' with 'strings.Contains'
S1004 Replace call to 'bytes.Compare' with 'bytes.Equal'
S1011 Use a single 'append' to concatenate two slices
S1012 Replace 'time.Now().Sub(x)' with 'time.Since(x)'
...

% staticcheck -explain S1003
Replace call to 'strings.Index' with 'strings.Contains'

As an added bonus, make -explain s1003 (lower case) work. Seemed too small of a change to make a new PR for.

@dominikh
Copy link
Owner

FWIW, \' is how we encode backticks (for Markdown) in raw string literals.

All the single quotes are written as `\'` in the descriptions. I assume
that's for a good reason, but -list-checks would print:

    % staticcheck -list-checks | grep "'"
    S1003 Replace call to \'strings.Index\' with \'strings.Contains\'
    S1004 Replace call to \'bytes.Compare\' with \'bytes.Equal\'
    S1011 Use a single \'append\' to concatenate two slices
    S1012 Replace \'time.Now().Sub(x)\' with \'time.Since(x)\'
    ...

    % staticcheck -explain S1003
    Replace call to \'strings.Index\' with \'strings.Contains\'

So fix that:

    % staticcheck -list-checks | grep "'"
    S1003 Replace call to 'strings.Index' with 'strings.Contains'
    S1004 Replace call to 'bytes.Compare' with 'bytes.Equal'
    S1011 Use a single 'append' to concatenate two slices
    S1012 Replace 'time.Now().Sub(x)' with 'time.Since(x)'
    ...

    % staticcheck -explain S1003
    Replace call to 'strings.Index' with 'strings.Contains'
@arp242
Copy link
Contributor Author

arp242 commented May 27, 2024

As an added bonus, make -explain s1003 (lower case) work. Seemed too small of a change to make a new PR for.

I backed out this change, as per #1546. As I mentioned there, I thought all CLI flags were case-insensitive but they're not.

I also added a commit to print fewer extraneous newlines in the Before/After blocks; see the commit message.

The Before and After had extraneous newlines; before:

    % staticcheck -explain S1000
    Use plain channel send or receive instead of single-case select

    Select statements with a single case can be replaced with a simple
    send or receive.

    Before (· represents blank line, git strips double blanks otherwise):
        ·
        ·
        select {
        case x := <-ch:
            fmt.Println(x)
        }
        ·
    After:
        ·
        ·
        x := <-ch
        fmt.Println(x)
        ·
        ·
    Available since
        2017.1

And after:

    % staticcheck -explain S1000
    Use plain channel send or receive instead of single-case select

    Select statements with a single case can be replaced with a simple
    send or receive.

    Before:

        select {
        case x := <-ch:
            fmt.Println(x)
        }

    After:

        x := <-ch
        fmt.Println(x)

    Available since
        2017.1
@dominikh
Copy link
Owner

dominikh commented Jun 3, 2024

I believe this is part of a broader regression introduced by 8643e6e where we broke the conversion of documentation to plain text and Markdown variants (cf. the use of lint.RawDocumentation and lint.Markdownify.) I'll look into fixing this myself.

@dominikh dominikh closed this in ac76929 Jun 3, 2024
@arp242
Copy link
Contributor Author

arp242 commented Jun 4, 2024

Yeah, I also discovered that on Sunday when I wanted to build the website. Fixing the website/cmd/generate_checks program wasn't too much work, but Hugo gives some error on shortcodes, and gave up at that.

@arp242 arp242 deleted the bsq branch June 4, 2024 23:13
@dominikh
Copy link
Owner

dominikh commented Jun 5, 2024

Yeah, I'll have to fix website soon. Hugo has been "fun" when it comes to backwards compatibility.

@arp242
Copy link
Contributor Author

arp242 commented Jun 5, 2024

Hugo has been "fun" when it comes to backwards compatibility.

I actually tried older versions of Hugo too, the current release from when the website was first added, and a release from a few months before that, but all gave the same error as the latest version:

% hugo
Start building sites …
hugo v0.126.2+extended linux/amd64 BuildDate=unknown

Total in 13 ms
Error: error building site: process: readAndProcessContent:
"/home/martin/src/go-tools/website/content/contact.md:9:1":
failed to extract shortcode: template for shortcode "blocks/section" not found

That was with v0.88.1 and v0.80.0, according to my shell history.

According to the Hugo docs everything looks fine too, so idk. At this point I gave up.

FYI.

@dominikh
Copy link
Owner

dominikh commented Jun 5, 2024

Make sure you've cloned the repository recursively; the website theme is pulled in via a git submodule.

@arp242
Copy link
Contributor Author

arp242 commented Jun 5, 2024

Ah right, that furthers things! Got fresh errors though.

I just wanted to look at some of the documentation issues in the weekend, and make sure whatever I wrote appears well on the website. Well, whatever 🤷

The amount of complexity involved here for rendering a few basic HTML pages is slightly ridiculous to be honest.

This pull request was closed.
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 this pull request may close these issues.

2 participants