Skip to content
This repository has been archived by the owner on Mar 4, 2022. It is now read-only.

Delete multiple file specification and dir-mode #161

Merged
merged 5 commits into from
Jul 31, 2018
Merged

Delete multiple file specification and dir-mode #161

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
143cd76
Delete dir-mode tests passing
bufdev Jul 30, 2018
7ba09b5
Finish delete dir-mode
bufdev Jul 30, 2018
62efc6f
Fix format
bufdev Jul 30, 2018
f0ba1ff
Fix format
bufdev Jul 30, 2018
29c8176
Address PR comments
bufdev Jul 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## [Unreleased]
- No changes yet.
- Delete the ability to explicitly specify multiple files, and have the effect
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be under a ### Removed header

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really Changed, since it's remove multiple files, have single file be old dir mode.

of one file being specified be the same as the former `--dir-mode`. See
[#16](https://github.com/uber/prototool/issues/16) for more details.


## [0.5.0] - 2018-07-26
Expand Down
17 changes: 6 additions & 11 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,22 +103,17 @@ $ prototool help lint
Lint proto files and compile with protoc to check for failures.

Usage:
prototool lint dirOrProtoFiles... [flags]

Flags:
--dir-mode Run as if the directory the file was given, but only print the errors from the file. Useful for integration with editors.
prototool lint [dirOrFile] [flags]
```

`dirOrProtoFiles...` can take multiple forms:
`dirOrFile` can take two forms:

- You can specify multiple files. If this is done, these files will be explicitly used for `protoc` calls.
- You can specify exactly one directory. If this is done, Prototool goes up until it finds a `prototool.yaml` file (or uses the current directory if none is found), and then walks starting at this location for all `.proto` files, and these are used, except for files in the `excludes` lists in `prototool.yaml` files.
- You can specify exactly one file, along with `--dir-mode`. This has the effect as if you specified the directory of this file (using the logic above), but errors are only printed for that file. This is useful for e.g. Vim integration.
- You can specify exactly one file. This has the effect as if you specified the directory of this file (using the logic above), but errors are only printed for that file. This is useful for e.g. Vim integration.
- You can specify nothing. This has the effect as if you specified the current directory as the directory.

The idea with "directory builds" is that you often need more than just one file to do a `protoc` call, for example if you have types in other files in the same package that are not referenced by their fully-qualified name, and/or if you need to know what directories to specify with `-I` to `protoc` (by default, the directory of the `prototool.yaml` file is used).

In general practice, directory builds are what you always want to do. File builds were just added for convenience, and [may be removed](https://github.com/uber/prototool/issues/16).

## Command Overview

Let's go over some of the basic commands. There are more commands than listed here, and [some may be removed before v1.0](https://github.com/uber/prototool/issues/11), but the following commands are what you mostly need to know.
Expand Down Expand Up @@ -215,7 +210,7 @@ If [Vim integration](#vim-integration) is set up, files will be generated when y

##### `prototool files`

Print the list of all files that will be used given the input `dirOrProtoFiles...`. Useful for debugging.
Print the list of all files that will be used given the input `dirOrFile`. Useful for debugging.

##### `prototool grpc`

Expand All @@ -234,7 +229,7 @@ There is a full example for gRPC in the [example](example) directory. Run `make

Start the example server in a separate terminal by doing `go run example/cmd/excited/main.go`.

`prototool grpc dirOrProtoFiles... --address serverAddress --method package.service/Method --data 'requestData'`
`prototool grpc [dirOrFile] --address serverAddress --method package.service/Method --data 'requestData'`

Either use `--data 'requestData'` as the the JSON data to input, or `--stdin` which will result in the input being read from stdin as JSON.

Expand Down
62 changes: 30 additions & 32 deletions internal/cmd/cmd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,53 +54,51 @@ func TestCompile(t *testing.T) {
assertDoCompileFiles(
t,
false,
`testdata/compile/dep_errors.proto:6:1:Expected ";".`,
"testdata/compile/dep_errors.proto",
`testdata/compile/errors_on_import/dep_errors.proto:6:1:Expected ";".`,
"testdata/compile/errors_on_import/dep_errors.proto",
)
assertDoCompileFiles(
t,
false,
`dep_errors.proto:6:1:Expected ";".
testdata/compile/errors_on_import.proto:10:3:"foo.DepError" is not defined.`,
"testdata/compile/errors_on_import.proto",
`testdata/compile/errors_on_import/dep_errors.proto:6:1:Expected ";".`,
"testdata/compile/errors_on_import",
)
assertDoCompileFiles(
t,
false,
`testdata/compile/extra_import.proto:1:1:Import "dep.proto" was not used.`,
"testdata/compile/extra_import.proto",
`testdata/compile/extra_import/extra_import.proto:1:1:Import "dep.proto" was not used.`,
"testdata/compile/extra_import/extra_import.proto",
)
assertDoCompileFiles(
t,
false,
`testdata/compile/json_camel_case_conflict.proto:1:1:The JSON camel-case name of field "helloworld" conflicts with field "helloWorld". This is not allowed in proto3.`,
"testdata/compile/json_camel_case_conflict.proto",
`testdata/compile/json/json_camel_case_conflict.proto:1:1:The JSON camel-case name of field "helloworld" conflicts with field "helloWorld". This is not allowed in proto3.`,
"testdata/compile/json/json_camel_case_conflict.proto",
)
assertDoCompileFiles(
t,
false,
`testdata/compile/missing_package_semicolon.proto:5:1:Expected ";".`,
"testdata/compile/missing_package_semicolon.proto",
`testdata/compile/semicolon/missing_package_semicolon.proto:5:1:Expected ";".`,
"testdata/compile/semicolon/missing_package_semicolon.proto",
)
assertDoCompileFiles(
t,
false,
`testdata/compile/missing_syntax.proto:1:1:No syntax specified. Please use 'syntax = "proto2";' or 'syntax = "proto3";' to specify a syntax version.
testdata/compile/missing_syntax.proto:4:3:Expected "required", "optional", or "repeated".`,
"testdata/compile/missing_syntax.proto",
`testdata/compile/syntax/missing_syntax.proto:1:1:No syntax specified. Please use 'syntax = "proto2";' or 'syntax = "proto3";' to specify a syntax version.
testdata/compile/syntax/missing_syntax.proto:4:3:Expected "required", "optional", or "repeated".`,
"testdata/compile/syntax/missing_syntax.proto",
)
assertDoCompileFiles(
t,
true,
``,
"testdata/compile/syntax_proto2.proto",
"testdata/compile/proto2/syntax_proto2.proto",
)
assertDoCompileFiles(
t,
false,
`testdata/compile/not_imported.proto:11:3:"foo.Dep" seems to be defined in "dep.proto", which is not imported by "not_imported.proto". To use it here, please add the necessary import.`,
"testdata/compile/dep.proto",
"testdata/compile/not_imported.proto",
`testdata/compile/notimported/not_imported.proto:11:3:"foo.Dep" seems to be defined in "dep.proto", which is not imported by "not_imported.proto". To use it here, please add the necessary import.`,
"testdata/compile/notimported/not_imported.proto",
)
}

Expand Down Expand Up @@ -130,13 +128,13 @@ func TestLint(t *testing.T) {
t,
false,
"1:1:SYNTAX_PROTO3",
"testdata/lint/syntax_proto2.proto",
"testdata/lint/syntaxproto2/syntax_proto2.proto",
)
assertDoLintFile(
t,
false,
"11:1:MESSAGE_NAMES_CAPITALIZED",
"testdata/lint/message_name_not_capitalized.proto",
"testdata/lint/capitalized/message_name_not_capitalized.proto",
)
assertDoLintFile(
t,
Expand All @@ -145,7 +143,7 @@ func TestLint(t *testing.T) {
1:1:FILE_OPTIONS_REQUIRE_JAVA_MULTIPLE_FILES
1:1:FILE_OPTIONS_REQUIRE_JAVA_OUTER_CLASSNAME
1:1:FILE_OPTIONS_REQUIRE_JAVA_PACKAGE`,
"testdata/lint/file_options_required.proto",
"testdata/lint/required/file_options_required.proto",
)
assertDoLintFile(
t,
Expand All @@ -155,7 +153,7 @@ func TestLint(t *testing.T) {
1:1:FILE_OPTIONS_REQUIRE_JAVA_OUTER_CLASSNAME
1:1:FILE_OPTIONS_REQUIRE_JAVA_PACKAGE
1:1:PACKAGE_IS_DECLARED`,
"testdata/lint/base_file.proto",
"testdata/lint/base/base_file.proto",
)
assertDoLintFile(
t,
Expand All @@ -164,7 +162,7 @@ func TestLint(t *testing.T) {
6:1:FILE_OPTIONS_EQUAL_JAVA_MULTIPLE_FILES_TRUE
7:1:FILE_OPTIONS_EQUAL_JAVA_OUTER_CLASSNAME_PROTO_SUFFIX
8:1:FILE_OPTIONS_EQUAL_JAVA_PACKAGE_COM_PREFIX`,
"testdata/lint/file_options_incorrect.proto",
"testdata/lint/fileoptions/file_options_incorrect.proto",
)
assertDoLintFiles(
t,
Expand Down Expand Up @@ -232,7 +230,7 @@ func TestLint(t *testing.T) {
98:3:ENUMS_NO_ALLOW_ALIAS
108:5:ENUMS_NO_ALLOW_ALIAS
`,
"testdata/lint/lots.proto",
"testdata/lint/lots/lots.proto",
)
assertDoLintFile(
t,
Expand Down Expand Up @@ -331,16 +329,16 @@ func TestLint(t *testing.T) {
1:1:FILE_OPTIONS_REQUIRE_JAVA_MULTIPLE_FILES
1:1:FILE_OPTIONS_REQUIRE_JAVA_OUTER_CLASSNAME
1:1:FILE_OPTIONS_REQUIRE_JAVA_PACKAGE`,
"testdata/lint/package_starts_with_keyword.proto",
"testdata/lint/keyword/package_starts_with_keyword.proto",
)
}

func TestGoldenFormat(t *testing.T) {
t.Parallel()
assertGoldenFormat(t, false, false, "testdata/format/bar/bar.proto")
assertGoldenFormat(t, false, false, "testdata/format/bar/bar_proto2.proto")
assertGoldenFormat(t, false, false, "testdata/format/foo/foo.proto")
assertGoldenFormat(t, false, false, "testdata/format/foo/foo_proto2.proto")
assertGoldenFormat(t, false, false, "testdata/format/proto3/foo/bar/bar.proto")
assertGoldenFormat(t, false, false, "testdata/format/proto2/foo/bar/bar_proto2.proto")
assertGoldenFormat(t, false, false, "testdata/format/proto3/foo/foo.proto")
assertGoldenFormat(t, false, false, "testdata/format/proto2/foo/foo_proto2.proto")
assertGoldenFormat(t, false, true, "testdata/format-rewrite/foo.proto")
}

Expand Down Expand Up @@ -867,9 +865,9 @@ func (s *excitedServer) ExclamationBidiStream(streamServer grpcpb.ExcitedService

func assertDoInternal(t *testing.T, stdin io.Reader, expectedExitCode int, expectedLinePrefixes string, args ...string) {
stdout, exitCode := testDoStdin(t, stdin, args...)
assert.Equal(t, expectedExitCode, exitCode)
expectedLinePrefixesSplit := getCleanLines(expectedLinePrefixes)
outputSplit := getCleanLines(stdout)
assert.Equal(t, expectedExitCode, exitCode, strings.Join(outputSplit, "\n"))
expectedLinePrefixesSplit := getCleanLines(expectedLinePrefixes)
require.Equal(t, len(expectedLinePrefixesSplit), len(outputSplit), strings.Join(outputSplit, "\n"))
for i, expectedLinePrefix := range expectedLinePrefixesSplit {
assert.True(t, strings.HasPrefix(outputSplit[i], expectedLinePrefix), "%s %d %s", expectedLinePrefix, i, strings.Join(outputSplit, "\n"))
Expand All @@ -895,7 +893,7 @@ func testDoInternal(stdin io.Reader, args ...string) (string, int) {
}
buffer := bytes.NewBuffer(nil)
// develMode is on, so we have access to all commands
exitCode := do(true, args, stdin, buffer, os.Stderr)
exitCode := do(true, args, stdin, buffer, buffer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to use the same buffer for both stdout and stderr? Seems fine in a test - just want to make sure that's what you intend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I took this out before, but as the tests operate on the output, I want to check that ALL output is as I expect (which is generally that stderr has no output)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for clarifying!

return strings.TrimSpace(buffer.String()), exitCode
}

Expand Down
5 changes: 0 additions & 5 deletions internal/cmd/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ type flags struct {
data string
debug bool
diffMode bool
dirMode bool
disableFormat bool
disableLint bool
dryRun bool
Expand Down Expand Up @@ -80,10 +79,6 @@ func (f *flags) bindDiffMode(flagSet *pflag.FlagSet) {
flagSet.BoolVarP(&f.diffMode, "diff", "d", false, "Write a diff instead of writing the formatted file to stdout.")
}

func (f *flags) bindDirMode(flagSet *pflag.FlagSet) {
flagSet.BoolVar(&f.dirMode, "dir-mode", false, "Run as if the directory the file was given, but only print the errors from the file. Useful for integration with editors.")
}

func (f *flags) bindDisableFormat(flagSet *pflag.FlagSet) {
flagSet.BoolVar(&f.disableFormat, "disable-format", false, "Do not run formatting.")
}
Expand Down
Loading