Introduce a -tables option. #45

bolinfest · 2017-01-30T07:48:50Z

The -tables option makes it possible to specify a JSON file whose
properties define arguments that will be passed to the
tables.OverrideTables() function such that other projects can define
their own constraints on argument formatting. This is valuable to
Buck or Skylark users that define rules that need to be formatted in a
way that is different from buildifier's defaults.

Test Plan:

Added a test: tables/jsonParser_test.go.
Verified that bazel test //... succeeds.
Created a JSON config for Buck and verified the output of the
following was what I expected when run from the Buck repo:

export BUILDIFIER_DIFF="diff -u"
find . -name BUCK | xargs -I {} ../buildifier/bazel-bin/buildifier/buildifier -buildifier_disable=label -tables=buildifier.json -d {}

For this test, the contents of buildifier.json were:

{
  "IsLabelArg": {
  },
  "LabelBlacklist": {
  },
  "IsSortableListArg": {
    "deps": true,
    "exported_deps": true,
    "provided_deps": true,
    "srcs": true,
    "visibility": true
  },
  "SortableBlacklist": {
    "genrule.srcs": true
  },
  "SortableWhitelist": {
  }
}

bazel-io · 2017-01-30T07:48:52Z

Can one of the admins verify this patch?

pmbethe09

LGTM, just a few nits

pmbethe09 · 2017-01-30T15:09:04Z

buildifier/buildifier.go

@@ -113,6 +115,14 @@ func main() {
 		os.Exit(2)
 	}

+	if *tbls != "" {
+		err := tables.ParseAndUpdateJsonDefinitions(*tbls)


push into if stmt

pmbethe09 · 2017-01-30T15:10:08Z

tables/jsonParser.go

@@ -0,0 +1,50 @@
+/*


Go-style is lowercase: name this jsonparser.go

OK, thanks, I wasn't sure because I saw there is a generateTables.go.

pmbethe09 · 2017-01-30T15:10:59Z

tables/jsonParser.go

+		return definitions, err
+	}
+
+	err = json.Unmarshal(data, &definitions)


return this err on the next line

Good catch: done.

pmbethe09 · 2017-01-30T15:11:33Z

tables/jsonParser.go

+func ParseAndUpdateJsonDefinitions(file string) error {
+	definitions, err := ParseJsonDefinitions(file)
+	if err == nil {
+		OverrideTables(definitions.IsLabelArg, definitions.LabelBlacklist, definitions.IsSortableListArg, definitions.SortableBlacklist, definitions.SortableWhitelist)


error guards are more normal:
if err != nil {
return err
}
happy flow...

pmbethe09 · 2017-01-30T15:12:00Z

tables/jsonParser_test.go

@@ -0,0 +1,42 @@
+/*


name this jsonparser_test.go

Renamed (always fun on OS X).

pmbethe09 · 2017-01-30T15:12:30Z

buildifier/buildifier.go

@@ -39,6 +40,7 @@ var (
 	dflag = flag.Bool("d", false, "alias for -mode=diff")
 	mode  = flag.String("mode", "", "formatting mode: check, diff, or fix (default fix)")
 	path  = flag.String("path", "", "assume BUILD file has this path relative to the workspace directory")
+	tbls  = flag.String("tables", "", "path to JSON file with custom table definitions")


s/tbls/tables/

I concede tbls is a terrible name, but I put this up initially for two reasons:

Because I'm importing github.com/bazelbuild/buildifier/tables above, I can't name it tables, can I?

If I name it tablesFlag or something like that, then am I supposed to update all of the lines above so the = signs line up?

Changed to tablesPath and re-indented.

pmbethe09 · 2017-01-30T15:12:59Z

Jenkins, test this please.

bolinfest

OK, just one question on variable naming.

bolinfest · 2017-01-30T18:25:40Z

@pmbethe09 All comments addressed. I hope it's OK that I squashed the commits. I assume that you'd rather just apply the final, resulting commit than merge in a small branch of intermediate steps? I realize different organizations have their own preferences (Facebook works this way, preferring rebases to merges).

pmbethe09 · 2017-01-30T19:35:51Z

buildifier/buildifier.go

@@ -113,6 +115,11 @@ func main() {
 		os.Exit(2)
 	}

+	if *tablesPath != "" && tables.ParseAndUpdateJsonDefinitions(*tablesPath) != nil {


grab the error from ParseAndUpdateJsonDefinitions and add to the Printf

OK. Note that this undoes the original change where you asked me to combine the two conditions into a single if. I don't know if there's some clever way to assign err in the if, though perhaps even if that's possible, it's not considered good style, so I did the simple thing.

pmbethe09 · 2017-01-30T19:36:04Z

tables/jsonparser.go

@@ -0,0 +1,52 @@
+/*
+Copyright 2016 Google Inc. All Rights Reserved.


pmbethe09 · 2017-01-30T19:36:08Z

tables/jsonparser_test.go

@@ -0,0 +1,42 @@
+/*
+Copyright 2016 Google Inc. All Rights Reserved.


pmbethe09 · 2017-01-30T19:37:23Z

tables/jsonparser_test.go

+		SortableWhitelist: map[string]bool{},
+	}
+	if !reflect.DeepEqual(expected, definitions) {
+		t.Error("structs not equal")


add structs to error message
t.Errorf("ParseJsonDefinitions() = %v; want %v", definitions, expected)

Thanks, I didn't know about %v. This is my first time writing Go, tbh.

bolinfest · 2017-01-30T19:53:57Z

@pmbethe09 OK, all comments addressed.

pmbethe09 · 2017-01-30T20:02:54Z

Jenkins, test this please.

pmbethe09 · 2017-01-30T20:03:18Z

buildifier/buildifier.go

@@ -113,6 +115,14 @@ func main() {
 		os.Exit(2)
 	}

+	if *tablesPath != "" {
+		err := tables.ParseAndUpdateJsonDefinitions(*tablesPath)


push into if

I'm not sure how that works. For example, this does not compile:

if *tablesPath != "" && (err := tables.ParseAndUpdateJsonDefinitions(*tablesPath)) != nil { fmt.Fprintf(os.Stderr, "buildifier: failed to parse %s for -tables: %s\n", *tablesPath, err) os.Exit(2) }

The if below, not above.
if *tablesPath != "" {
if err := tables.ParseAndUpdateJsonDefinitions(*tablesPath); err != nil {
...
}
}

IMHO. By pushing into if, you create a clear scope of set and inspect.

Ah, cool, I didn't know about that syntax. Yes, I agree that's nicer. Fixed.

The `-tables` option makes it possible to specify a JSON file whose properties define arguments that will be passed to the `tables.OverrideTables()` function such that other projects can define their own constraints on argument formatting. This is valuable to Buck or Skylark users that define rules that need to be formatted in a way that is different from buildifier's defaults. Test Plan: * Added a test: `tables/jsonParser_test.go`. * Verified that `bazel test //...` succeeds. * Created a JSON config for Buck and verified the output of the following was what I expected when run from the Buck repo: ``` export BUILDIFIER_DIFF="diff -u" find . -name BUCK | xargs -I {} ../buildifier/bazel-bin/buildifier/buildifier -buildifier_disable=label -tables=buildifier.json -d {} ``` For this test, the contents of `buildifier.json` were: ``` { "IsLabelArg": { }, "LabelBlacklist": { }, "IsSortableListArg": { "deps": true, "exported_deps": true, "provided_deps": true, "srcs": true, "visibility": true }, "SortableBlacklist": { "genrule.srcs": true }, "SortableWhitelist": { } } ```

bolinfest · 2017-01-30T20:16:19Z

Awesome, thanks so much for the review, @pmbethe09!

bolinfest mentioned this pull request Jan 30, 2017

Make indent and quote style configurable. #34

Open

pmbethe09 reviewed Jan 30, 2017

View reviewed changes

pmbethe09 assigned damienmg Jan 30, 2017

pmbethe09 mentioned this pull request Jan 30, 2017

Publish gh release binaries #44

Closed

bolinfest commented Jan 30, 2017

View reviewed changes

bolinfest force-pushed the custom-tables branch 2 times, most recently from 10d4284 to d268493 Compare January 30, 2017 18:15

pmbethe09 reviewed Jan 30, 2017

View reviewed changes

bolinfest force-pushed the custom-tables branch from d268493 to d6a9044 Compare January 30, 2017 19:51

pmbethe09 reviewed Jan 30, 2017

View reviewed changes

bolinfest force-pushed the custom-tables branch from d6a9044 to edd37b0 Compare January 30, 2017 20:14

pmbethe09 merged commit 451dae5 into bazelbuild:master Jan 30, 2017

bolinfest deleted the custom-tables branch January 30, 2017 20:21

This was referenced Jun 19, 2017

Install bot2-lcmgl RobotLocomotion/drake#6383

Merged

Improve table overrides #112

Closed

illicitonion mentioned this pull request Oct 6, 2017

Allow set literals in BUILD files #146

Merged

		@@ -0,0 +1,52 @@
		/*
		Copyright 2016 Google Inc. All Rights Reserved.

		@@ -0,0 +1,42 @@
		/*
		Copyright 2016 Google Inc. All Rights Reserved.

Introduce a -tables option. #45

Introduce a -tables option. #45

Conversation

bolinfest commented Jan 30, 2017

bazel-io commented Jan 30, 2017

pmbethe09 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmbethe09 commented Jan 30, 2017

bolinfest left a comment

Choose a reason for hiding this comment

bolinfest commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bolinfest commented Jan 30, 2017

pmbethe09 commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bolinfest commented Jan 30, 2017