🌟 Let's talk about output formats options

[ldez]

Important

This is a proposal: I don't know if it is possible and what the impact could be inside the code.
The proposal may evolve.

---

The current configuration

# colored-line-number
# line-number
# json
# colored-tab
# tab
# html
# checkstyle
# code-climate
# junit-xml
# junit-xml-extended
# github-actions
# teamcity
# sarif

output:
  print-issued-lines: false
  print-linter-name: false
  formats:
    - format: json
      path: stderr
    - format: checkstyle
      path: report.xml
    - format: colored-line-number

--out-format=checkstyle:report.xml,json:stdout,line-number-colored
--print-issued-lines=false
--print-linter-name=false

---

⛑️ The Problems

Global Options but Only For Some Formats

output:
  print-issued-lines: false
  print-linter-name: false
  formats:
    - format: json
      path: stderr
    - format: checkstyle
      path: report.xml
    - format: colored-line-number

• The option output.print-issued-lines is for line-number, colored-line-number formats.
• The option output.print-linter-name is for line-number, colored-line-number, tab, colored-tab formats.

Format Names

The names of the colored variants have an unexpected style:

• line-number, colored-line-number
• tab, colored-tab

I think tab, tab-colored are more natural.

Slice of Formats

output:
  formats:
    - format: json
      path: stderr
    - format: checkstyle
      path: report.xml
    - format: colored-line-number

Currently, the formats are represented into a slice, this type has been introduced mainly for compatibility reasons with the initial format option (a simple string)

A side effect of a slice is the possibility of defining the same format several times but with different output paths.
But I think this is not a real use case.

Another side effect is the flag parsing complexity because the flag --out-format value is a simple string.

Non-User friendly CLI flag

The current syntax can be used only if you know the syntax and there is no suggestion/completion.

--out-format=checkstyle:report.xml,json:stdout,line-number-colored

💭 Proposal

Configuration

output:
  formats:
    text: # (line-number)
      path: stdout
      print-linter-name: true
      print-issued-lines: true
      colors: true
    json:
      path: ./path/to/output.json
    tab:
      path: stdout
      print-linter-name: true
      colors: true
    html:
      path: ./path/to/output.html
    checkstyle:
      path: ./path/to/output.xml
    code-climate:
      path: ./path/to/output.json
    junit-xml:
      path: ./path/to/output.xml
      extended: true
    teamcity:
      path: ./path/to/output.txt
    sarif:
      path: ./path/to/output.json

CLI flags

--output.text.path=stdout
--output.text.print-linter-name=false
--output.text.print-issued-lines=false
--output.text.colors=false
--output.json.path=./path/to/output.json
--output.tab.path=stdout
--output.tab.print-linter-name=false
--output.tab.colors=false
--output.html.path=./path/to/output.html
--output.checkstyle.path=./path/to/output.xml
--output.code-climate.path=./path/to/output.json
--output.junit-xml.path=./path/to/output.xml
--output.junit-xml.extended=true
--output.teamcity.path=./path/to/output.txt
--output.sarif.path=./path/to/output.json

example

Before:

--out-format=checkstyle:report.xml,json:stdout,line-number-colored

After:

--output.checkstyle.path=report.xml
--output.json.path=stdout
--output.text.path=stdout

[ldez]

I created a working implementation.

As the main key output.formats is the same as the existing one, and the current implementation uses a hack to handle multiple types (string, slice), I didn't find a way to create a compatibility layer.

I wonder if the flag prefix --output. is right or not, I mean --output.text.path vs --text.path.

[ldez]

Maybe the new main key can be output.styles 🤔

With the new fmt command it can be better to avoid the word format about elements related to linters 🤔

[bombsimon]

I wonder if the flag prefix --output. is right or not, I mean --output.text.path vs --text.path.

Maybe the new main key can be output.styles 🤔

With the new fmt command it can be better to avoid the word format about elements related to linters 🤔

I think the proposal looks good and would be an improvement from the current --out-format. By supporting multiple output formats at the same time it can become complex and messy with the flags rather than just having the user pipe to a file and us printing to stdout. And as long as we want to support all configs from a config file as CLI flags it will look something like your suggestion.

I think it's fine to (re)use format if you want to, don't see it as risk of ambiguity with the fmt "format". I'd prefer it over styles at least.

But I also pondering if it would be possible to keep using out-format but improve the usage? Right now we don't have anything other than format at the second level in the config and it's completely omitted in the CLI flags so it's not a 1:1 map.

Thinking

---
out-format:
  json:
    path: /tmp/out.json
  tab:
    colors: true

--out-format.json.path=/tmp.out.json
--out-format.tab.colors=true

In your example in the original post it seems like line-number-colored got dropped, where would that go? Something like --output.line-number-colored?

[ldez]

line-number/line-number-colored -> text

line-number-colored (the default):

--output.text.path=stdout
--output.text.colors=true

line-number:

--output.text.path=stdout
--output.text.colors=false

[ldez]

But I also pondering if it would be possible to keep using out-format but improve the usage?

I'm not a fan of out-format: this is weird to use out instead of output.

I'd prefer it over styles at least.

Ok so I cannot create a compatibility layer, the PR will be open after the next minor release.

[ldez]

Note: I trimmed the configuration to only focus on formats but there are more fields.

output:
  path-prefix: ""
  sort-results: true
  sort-order:
    - linter
    - severity
    - file
  show-stats: true
  # ...

[bombsimon]

I'm not a fan of out-format: this is weird to use out instead of output.

Yeah, I agree. Not sure why I wrote like formats was the only section, given the current config structure output seems like the best name and format or formats as the second level.

[ldez]

FYI output.format is already used, it was the previous name of output.formats, and this is still used for compatibility.

This is why I said I cannot create a compatibility layer without a new name.

[Antonboom]

Do we really need compatibility layer here?

I confused that cli arg is not consistent with config tree

output.text.path vs output.formats.text.path 🤔

[ldez]

Do we really need compatibility layer here?

If we merge this inside v1, we need a compatibility layer.

But as a compatibility layer is not possible, it will be merged in v2 only.

I confused that cli arg is not consistent with config tree

99% of the current CLI flags are not "consistent", and it's for the better because the goal is not to provide a mapping of the configuration as flags, but to provide some user-friendly flags only for a few options.

Flags:
  -c, --config PATH                    Read config from file path PATH
      --no-config                      Don't read config file
  -D, --disable strings                Disable specific linter
      --disable-all                    Disable all linters
  -E, --enable strings                 Enable specific linter
      --enable-all                     Enable all linters
      --fast                           Enable only fast linters from enabled linters set (first run won't be fast)
  -p, --presets strings                Enable presets of linters:
                                         - bugs
                                         - comment
                                         - complexity
                                         - error
                                         - format
                                         - import
                                         - metalinter
                                         - module
                                         - performance
                                         - sql
                                         - style
                                         - test
                                         - unused
                                       Run 'golangci-lint help linters' to see them.
                                       This option implies option --disable-all
      --enable-only strings            Override linters configuration section to only run the specific linter(s)
  -j, --concurrency int                Number of CPUs to use (Default: number of logical CPUs) (default 16)
      --modules-download-mode string   Modules download mode. If not empty, passed as -mod=<mode> to go tools
      --issues-exit-code int           Exit code when issues were found (default 1)
      --go string                      Targeted Go version
      --build-tags strings             Build tags
      --timeout duration               Timeout for total work. If <= 0, the timeout is disabled (default 1m0s)
      --tests                          Analyze tests (*_test.go) (default true)
      --allow-parallel-runners         Allow multiple parallel golangci-lint instances running.
                                       If false (default) - golangci-lint acquires file lock on start.
      --allow-serial-runners           Allow multiple golangci-lint instances running, but serialize them around a lock.
                                       If false (default) - golangci-lint exits with an error if it fails to acquire file lock on start.
      --out-format string              Formats of output:
                                         - json
                                         - line-number
                                         - colored-line-number
                                         - tab
                                         - colored-tab
                                         - checkstyle
                                         - code-climate
                                         - html
                                         - junit-xml
                                         - junit-xml-extended
                                         - github-actions
                                         - teamcity
                                         - sarif
                                        (default "colored-line-number")
      --print-issued-lines             Print lines of code with issue (default true)
      --print-linter-name              Print linter name in issue line (default true)
      --sort-results                   Sort linter results
      --sort-order strings             Sort order of linter results
      --path-prefix string             Path prefix to add to output
      --show-stats                     Show statistics per linter
  -e, --exclude strings                Exclude issue by regexp
      --exclude-use-default            Use or not use default excludes:
                                         - EXC0001 (errcheck): Almost all programs ignore errors on these functions and in most cases it's ok.
                                           Pattern: 'Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked'
                                         - EXC0002 (golint): Annoying issue about not having a comment. The rare codebase has such comments.
                                           Pattern: '(comment on exported (method|function|type|const)|should have( a package)? comment|comment should be of the form)'
                                         - EXC0003 (golint): False positive when tests are defined in package 'test'.
                                           Pattern: 'func name will be used as test\.Test.* by other packages, and that stutters; consider calling this'
                                         - EXC0004 (govet): Common false positives.
                                           Pattern: '(possible misuse of unsafe.Pointer|should have signature)'
                                         - EXC0005 (staticcheck): Developers tend to write in C-style with an explicit 'break' in a 'switch', so it's ok to ignore.
                                           Pattern: 'SA4011'
                                         - EXC0006 (gosec): Too many false-positives on 'unsafe' usage.
                                           Pattern: 'G103: Use of unsafe calls should be audited'
                                         - EXC0007 (gosec): Too many false-positives for parametrized shell calls.
                                           Pattern: 'G204: Subprocess launched with variable'
                                         - EXC0008 (gosec): Duplicated errcheck checks.
                                           Pattern: 'G104'
                                         - EXC0009 (gosec): Too many issues in popular repos.
                                           Pattern: '(G301|G302|G307): Expect (directory permissions to be 0750|file permissions to be 0600) or less'
                                         - EXC0010 (gosec): False positive is triggered by 'src, err := ioutil.ReadFile(filename)'.
                                           Pattern: 'G304: Potential file inclusion via variable'
                                         - EXC0011 (stylecheck): Annoying issue about not having a comment. The rare codebase has such comments.
                                           Pattern: '(ST1000|ST1020|ST1021|ST1022)'
                                         - EXC0012 (revive): Annoying issue about not having a comment. The rare codebase has such comments.
                                           Pattern: 'exported (.+) should have comment( \(or a comment on this block\))? or be unexported'
                                         - EXC0013 (revive): Annoying issue about not having a comment. The rare codebase has such comments.
                                           Pattern: 'package comment should be of the form "(.+)..."'
                                         - EXC0014 (revive): Annoying issue about not having a comment. The rare codebase has such comments.
                                           Pattern: 'comment on exported (.+) should be of the form "(.+)..."'
                                         - EXC0015 (revive): Annoying issue about not having a comment. The rare codebase has such comments.
                                           Pattern: 'should have a package comment' (default true)
      --exclude-case-sensitive         If set to true exclude and exclude rules regular expressions are case-sensitive
      --max-issues-per-linter int      Maximum issues count per one linter. Set to 0 to disable (default 50)
      --max-same-issues int            Maximum count of issues with the same text. Set to 0 to disable (default 3)
      --uniq-by-line                   Make issues output unique by line (default true)
      --exclude-files strings          Regexps of files to exclude
      --exclude-dirs strings           Regexps of directories to exclude
      --exclude-dirs-use-default       Use or not use default excluded directories:
                                         - (^|/)vendor($|/)
                                         - (^|/)third_party($|/)
                                         - (^|/)testdata($|/)
                                         - (^|/)examples($|/)
                                         - (^|/)Godeps($|/)
                                         - (^|/)builtin($|/)
                                        (default true)
      --exclude-generated string       Mode of the generated files analysis (default "lax")
  -n, --new                            Show only new issues: if there are unstaged changes or untracked files, only those changes are analyzed, else only changes in HEAD~ are analyzed.
                                       It's a super-useful option for integration of golangci-lint into existing large codebase.
                                       It's not practical to fix all existing issues at the moment of integration: much better to not allow issues in new code.
                                       For CI setups, prefer --new-from-rev=HEAD~, as --new can skip linting the current patch if any scripts generate unstaged files before golangci-lint runs.
      --new-from-rev REV               Show only new issues created after git revision REV
      --new-from-patch PATH            Show only new issues created in git patch with file path PATH
      --whole-files                    Show issues in any part of update files (requires new-from-rev or new-from-patch)
      --fix                            Fix found issues (if it's supported by the linter)
      --cpu-profile-path string        Path to CPU profile output file
      --mem-profile-path string        Path to memory profile output file
      --print-resources-usage          Print avg and max memory usage of golangci-lint and total time
      --trace-path string              Path to trace output file