-
Notifications
You must be signed in to change notification settings - Fork 111
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat: Add native linter and formatter (#522)
* feat: Add lint command to pixlet. This commit adds a lint command to pixlet that wraps buildifier. Right now, it works as a drop in replacement for buildifier, but it's not very user friendly from a pixlet perspective. I'll follow up with another commit to make this easier to use from a pixlet lens. * feat: Break apart lint and format. This commit splits linting and formatting into two commands and cleans up all of the buildifier flags so that it's easier to use in pixlet. * Clean up lint command and interface. This commit cleans up the lint command, help, and examples to behave as discussed in Discord. * Clean up format command and interface. This commit cleans up the format command and interface to behave has we disscussed in Discord. * Update format to output. When I was typing these commands, I felt the urge to add `-o json` instead of --format. Given it felt more natural to me, I updated the command arg. * Fix warnings list to include all warnings. This commit updates the linter to include all warnings. I originally left this out by mistake, so this is a bug fix. We may consider a different list here in the future but it will require more testing. * Drop output for format. The output format doesn't really make sense for formatting. As a pixlet user, I only care about formatting or a diff. The json format only spits out the files processed, which I don't really care about at this time. * Fix formatting when fix flag is set. When linting, we need to fix the formatting in order to also fix the resolvable lint issues. This commit ensures we do both if the --fix flag is set. * Set default warning list. This commit sets up a default warning list with a disabledWarnings map to be able to control which warnings we enable.
- Loading branch information
1 parent
5b78d95
commit 41c811f
Showing
6 changed files
with
394 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,278 @@ | ||
| /* | ||
| Copyright 2016 Google LLC | ||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
| https://www.apache.org/licenses/LICENSE-2.0 | ||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package cmd | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "fmt" | ||
| "io" | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" | ||
|
|
||
| "github.com/bazelbuild/buildtools/build" | ||
| "github.com/bazelbuild/buildtools/buildifier/utils" | ||
| "github.com/bazelbuild/buildtools/differ" | ||
| "github.com/bazelbuild/buildtools/warn" | ||
| "github.com/bazelbuild/buildtools/wspace" | ||
| ) | ||
|
|
||
| var ( | ||
| vflag bool | ||
| rflag bool | ||
| dryRunFlag bool | ||
| fixFlag bool | ||
| outputFormat string | ||
| ) | ||
|
|
||
| func runBuildifier(args []string, lint string, mode string, format string, recursive bool, verbose bool) int { | ||
| tf := &utils.TempFile{} | ||
| defer tf.Clean() | ||
|
|
||
| exitCode := 0 | ||
| var diagnostics *utils.Diagnostics | ||
| if len(args) == 0 || (len(args) == 1 && (args)[0] == "-") { | ||
| // Read from stdin, write to stdout. | ||
| data, err := io.ReadAll(os.Stdin) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "buildifier: reading stdin: %v\n", err) | ||
| return 2 | ||
| } | ||
| if mode == "fix" { | ||
| mode = "pipe" | ||
| } | ||
| var fileDiagnostics *utils.FileDiagnostics | ||
| fileDiagnostics, exitCode = processFile("", data, lint, false, tf, mode, verbose) | ||
| diagnostics = utils.NewDiagnostics(fileDiagnostics) | ||
| } else { | ||
| files := args | ||
| if recursive { | ||
| var err error | ||
| files, err = utils.ExpandDirectories(&args) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "buildifier: %v\n", err) | ||
| return 3 | ||
| } | ||
| } | ||
| diagnostics, exitCode = processFiles(files, lint, tf, mode, verbose) | ||
| } | ||
|
|
||
| diagnosticsOutput := diagnostics.Format(format, verbose) | ||
| if format != "" { | ||
| // Explicitly provided --format means the diagnostics are printed to stdout | ||
| fmt.Printf(diagnosticsOutput) | ||
| // Exit code should be set to 0 so that other tools know they can safely parse the json | ||
| exitCode = 0 | ||
| } else { | ||
| // --format is not provided, stdout is reserved for file contents | ||
| fmt.Fprint(os.Stderr, diagnosticsOutput) | ||
| } | ||
|
|
||
| if err := diff.Run(); err != nil { | ||
| fmt.Fprintf(os.Stderr, "%v\n", err) | ||
| return 2 | ||
| } | ||
|
|
||
| return exitCode | ||
| } | ||
|
|
||
| func processFiles(files []string, lint string, tf *utils.TempFile, mode string, verbose bool) (*utils.Diagnostics, int) { | ||
| // Decide how many file reads to run in parallel. | ||
| // At most 100, and at most one per 10 input files. | ||
| nworker := 100 | ||
| if n := (len(files) + 9) / 10; nworker > n { | ||
| nworker = n | ||
| } | ||
| runtime.GOMAXPROCS(nworker + 1) | ||
|
|
||
| // Start nworker workers reading stripes of the input | ||
| // argument list and sending the resulting data on | ||
| // separate channels. file[k] is read by worker k%nworker | ||
| // and delivered on ch[k%nworker]. | ||
| type result struct { | ||
| file string | ||
| data []byte | ||
| err error | ||
| } | ||
|
|
||
| ch := make([]chan result, nworker) | ||
| for i := 0; i < nworker; i++ { | ||
| ch[i] = make(chan result, 1) | ||
| go func(i int) { | ||
| for j := i; j < len(files); j += nworker { | ||
| file := files[j] | ||
| data, err := os.ReadFile(file) | ||
| ch[i] <- result{file, data, err} | ||
| } | ||
| }(i) | ||
| } | ||
|
|
||
| exitCode := 0 | ||
| fileDiagnostics := []*utils.FileDiagnostics{} | ||
|
|
||
| // Process files. The processing still runs in a single goroutine | ||
| // in sequence. Only the reading of the files has been parallelized. | ||
| // The goal is to optimize for runs where most files are already | ||
| // formatted correctly, so that reading is the bulk of the I/O. | ||
| for i, file := range files { | ||
| res := <-ch[i%nworker] | ||
| if res.file != file { | ||
| fmt.Fprintf(os.Stderr, "buildifier: internal phase error: got %s for %s", res.file, file) | ||
| os.Exit(3) | ||
| } | ||
| if res.err != nil { | ||
| fmt.Fprintf(os.Stderr, "buildifier: %v\n", res.err) | ||
| exitCode = 3 | ||
| continue | ||
| } | ||
| fd, newExitCode := processFile(file, res.data, lint, len(files) > 1, tf, mode, verbose) | ||
| if fd != nil { | ||
| fileDiagnostics = append(fileDiagnostics, fd) | ||
| } | ||
| if newExitCode != 0 { | ||
| exitCode = newExitCode | ||
| } | ||
| } | ||
| return utils.NewDiagnostics(fileDiagnostics...), exitCode | ||
| } | ||
|
|
||
| // diff is the differ to use when *mode == "diff". | ||
| var diff *differ.Differ | ||
|
|
||
| func defaultWarnings() []string { | ||
| warnings := []string{} | ||
| for _, warning := range warn.AllWarnings { | ||
| if !disabledWarnings[warning] { | ||
| warnings = append(warnings, warning) | ||
| } | ||
| } | ||
| return warnings | ||
| } | ||
|
|
||
| var disabledWarnings = map[string]bool{ | ||
| "function-docstring": true, // disables docstring warnings | ||
| "function-docstring-header": true, // disables docstring warnings | ||
| "function-docstring-args": true, // disables docstring warnings | ||
| "function-docstring-return": true, // disables docstring warnings | ||
| "native-android": true, // disables native android rules | ||
| "native-cc": true, // disables native cc rules | ||
| "native-java": true, // disables native java rules | ||
| "native-proto": true, // disables native proto rules | ||
| "native-py": true, // disables native python rules | ||
| } | ||
|
|
||
| // processFile processes a single file containing data. | ||
| // It has been read from filename and should be written back if fixing. | ||
| func processFile(filename string, data []byte, lint string, displayFileNames bool, tf *utils.TempFile, mode string, verbose bool) (*utils.FileDiagnostics, int) { | ||
| var exitCode int | ||
|
|
||
| displayFilename := filename | ||
| parser := utils.GetParser("auto") | ||
|
|
||
| f, err := parser(displayFilename, data) | ||
| if err != nil { | ||
| // Do not use buildifier: prefix on this error. | ||
| // Since it is a parse error, it begins with file:line: | ||
| // and we want that to be the first thing in the error. | ||
| fmt.Fprintf(os.Stderr, "%v\n", err) | ||
| if exitCode < 1 { | ||
| exitCode = 1 | ||
| } | ||
| return utils.InvalidFileDiagnostics(displayFilename), exitCode | ||
| } | ||
|
|
||
| if absoluteFilename, err := filepath.Abs(displayFilename); err == nil { | ||
| f.WorkspaceRoot, f.Pkg, f.Label = wspace.SplitFilePath(absoluteFilename) | ||
| } | ||
|
|
||
| enabledWarnings := defaultWarnings() | ||
| warnings := utils.Lint(f, lint, &enabledWarnings, verbose) | ||
| if len(warnings) > 0 { | ||
| exitCode = 4 | ||
| } | ||
| fileDiagnostics := utils.NewFileDiagnostics(f.DisplayPath(), warnings) | ||
|
|
||
| ndata := build.Format(f) | ||
|
|
||
| switch mode { | ||
| case "check": | ||
| // check mode: print names of files that need formatting. | ||
| if !bytes.Equal(data, ndata) { | ||
| fileDiagnostics.Formatted = false | ||
| return fileDiagnostics, 4 | ||
| } | ||
|
|
||
| case "diff": | ||
| // diff mode: run diff on old and new. | ||
| if bytes.Equal(data, ndata) { | ||
| return fileDiagnostics, exitCode | ||
| } | ||
| outfile, err := tf.WriteTemp(ndata) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "buildifier: %v\n", err) | ||
| return fileDiagnostics, 3 | ||
| } | ||
| infile := filename | ||
| if filename == "" { | ||
| // data was read from standard filename. | ||
| // Write it to a temporary file so diff can read it. | ||
| infile, err = tf.WriteTemp(data) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "buildifier: %v\n", err) | ||
| return fileDiagnostics, 3 | ||
| } | ||
| } | ||
| if displayFileNames { | ||
| fmt.Fprintf(os.Stderr, "%v:\n", f.DisplayPath()) | ||
| } | ||
| if err := diff.Show(infile, outfile); err != nil { | ||
| fmt.Fprintf(os.Stderr, "%v\n", err) | ||
| return fileDiagnostics, 4 | ||
| } | ||
|
|
||
| case "pipe": | ||
| // pipe mode - reading from stdin, writing to stdout. | ||
| // ("pipe" is not from the command line; it is set above in main.) | ||
| os.Stdout.Write(ndata) | ||
|
|
||
| case "fix": | ||
| // fix mode: update files in place as needed. | ||
| if bytes.Equal(data, ndata) { | ||
| return fileDiagnostics, exitCode | ||
| } | ||
|
|
||
| err := os.WriteFile(filename, ndata, 0666) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "buildifier: %s\n", err) | ||
| return fileDiagnostics, 3 | ||
| } | ||
|
|
||
| if verbose { | ||
| fmt.Fprintf(os.Stderr, "fixed %s\n", f.DisplayPath()) | ||
| } | ||
| case "print_if_changed": | ||
| if bytes.Equal(data, ndata) { | ||
| return fileDiagnostics, exitCode | ||
| } | ||
|
|
||
| if _, err := os.Stdout.Write(ndata); err != nil { | ||
| fmt.Fprintf(os.Stderr, "buildifier: error writing output: %v\n", err) | ||
| return fileDiagnostics, 3 | ||
| } | ||
| } | ||
| return fileDiagnostics, exitCode | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| package cmd | ||
|
|
||
| import ( | ||
| "os" | ||
|
|
||
| "github.com/bazelbuild/buildtools/differ" | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| func init() { | ||
| FormatCmd.Flags().BoolVarP(&vflag, "verbose", "v", false, "print verbose information to standard error") | ||
| FormatCmd.Flags().BoolVarP(&rflag, "recursive", "r", false, "find starlark files recursively") | ||
| FormatCmd.Flags().BoolVarP(&dryRunFlag, "dry-run", "d", false, "display a diff of formatting changes without modification") | ||
| } | ||
|
|
||
| var FormatCmd = &cobra.Command{ | ||
| Use: "format <pathspec>...", | ||
| Short: "Formats Tidbyt apps", | ||
| Example: ` pixlet format app.star | ||
| pixlet format app.star --dry-run | ||
| pixlet format --recursive ./`, | ||
| Long: `The format command provides a code formatter for Tidbyt apps. By default, it | ||
| will format your starlark source code in line. If you wish you see the output | ||
| before applying, add the --dry-run flag.`, | ||
| Args: cobra.MinimumNArgs(1), | ||
| Run: formatCmd, | ||
| } | ||
|
|
||
| func formatCmd(cmd *cobra.Command, args []string) { | ||
| // Lint refers to the lint mode for buildifier, with the options being off, | ||
| // warn, or fix. For pixlet format, we don't want to lint at all. | ||
| lint := "off" | ||
|
|
||
| // Mode refers to formatting mode for buildifier, with the options being | ||
| // check, diff, or fix. For the pixlet format command, we want to fix the | ||
| // resolvable issue by default and provide a dry run flag to be able to | ||
| // diff the changes before fixing them. | ||
| mode := "fix" | ||
| if dryRunFlag { | ||
| mode = "diff" | ||
| } | ||
|
|
||
| // Copied from the buildifier source, we need to supply a diff program for | ||
| // the differ. | ||
| differ, _ := differ.Find() | ||
| diff = differ | ||
|
|
||
| // Run buildifier and exit with the returned exit code. | ||
| exitCode := runBuildifier(args, lint, mode, "text", rflag, vflag) | ||
| os.Exit(exitCode) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| package cmd | ||
|
|
||
| import ( | ||
| "os" | ||
|
|
||
| "github.com/bazelbuild/buildtools/differ" | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| func init() { | ||
| LintCmd.Flags().BoolVarP(&vflag, "verbose", "v", false, "print verbose information to standard error") | ||
| LintCmd.Flags().BoolVarP(&rflag, "recursive", "r", false, "find starlark files recursively") | ||
| LintCmd.Flags().BoolVarP(&fixFlag, "fix", "f", false, "automatically fix resolvable lint issues") | ||
| LintCmd.Flags().StringVarP(&outputFormat, "output", "o", "text", "output format, text or json") | ||
| } | ||
|
|
||
| var LintCmd = &cobra.Command{ | ||
| Use: "lint <pathspec>...", | ||
| Example: ` pixlet lint app.star | ||
| pixlet lint --recursive --fix ./`, | ||
| Short: "Lints Tidbyt apps", | ||
| Long: `The lint command provides a linter for Tidbyt apps. It's capable of linting a | ||
| file, a list of files, or directory with the recursive option. Additionally, it | ||
| provides an option to automatically fix resolvable linter issues.`, | ||
| Args: cobra.MinimumNArgs(1), | ||
| Run: lintCmd, | ||
| } | ||
|
|
||
| func lintCmd(cmd *cobra.Command, args []string) { | ||
| // Mode refers to formatting mode for buildifier, with the options being | ||
| // check, diff, or fix. For the pixlet lint command, we only want to check | ||
| // formatting. | ||
| mode := "check" | ||
|
|
||
| // Lint refers to the lint mode for buildifier, with the options being off, | ||
| // warn, or fix. For pixlet lint, we want to warn by default but offer a | ||
| // flag to automatically fix resolvable issues. | ||
| lint := "warn" | ||
|
|
||
| // If the fix flag is enabled, the lint command should both format and lint. | ||
| if fixFlag { | ||
| mode = "fix" | ||
| lint = "fix" | ||
| } | ||
|
|
||
| // Copied from the buildifier source, we need to supply a diff program for | ||
| // the differ. | ||
| differ, _ := differ.Find() | ||
| diff = differ | ||
|
|
||
| // TODO: We currently offer misspelling protection in the community repo | ||
| // for app manifests. We'll want to consider adding additional spelling | ||
| // support to pixlet lint to ensure typos in apps don't make it to | ||
| // production. | ||
|
|
||
| // Run buildifier and exit with the returned exit code. | ||
| exitCode := runBuildifier(args, lint, mode, outputFormat, rflag, vflag) | ||
| os.Exit(exitCode) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.