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.

Sign up for GitHub

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

Jump to bottom

Grpc.Tools document AdditionalImportDirs #30405

Merged
merged 4 commits into from Sep 2, 2022
Merged

Grpc.Tools document AdditionalImportDirs #30405

merged 4 commits into from Sep 2, 2022

Conversation

tonydnewell
Copy link
Contributor

Add documentation for AdditionalImportDirs
See #22878

@tonydnewell tonydnewell mentioned this pull request Jul 26, 2022
Closed
@jtattermusch jtattermusch self-requested a review August 16, 2022 12:27
@jtattermusch jtattermusch self-assigned this Aug 16, 2022
@jtattermusch jtattermusch added the release notes: yes Indicates if PR needs to be in release notes label Aug 16, 2022
jtattermusch
jtattermusch reviewed Aug 17, 2022
View reviewed changes
Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

Left a few comments to address, but otherwise looks good.

src/csharp/BUILD-INTEGRATION.md Outdated
@@ -389,12 +398,17 @@ and "GrpcServices" metadata, but it might be useful in situations where you want
to explicitly pass some otherwise unsupported (e.g. experimental) options to the
`grpc_csharp_plugin`.

* __AdditionalImportDirs__
Pass additional directories to include proto files. These may be directories _outside_ of the
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it's inaccurate to say "Pass additional directories to include proto files".
How about "Specify additional directories in which the protocol buffer compiler will search for imported proto files."?

FTR, the official helptext for the -I flag in protoc is : Specify the directory in which to search for imports. May be specified multiple times; directories will be searched in order. ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

src/csharp/BUILD-INTEGRATION.md Outdated
@@ -352,6 +360,7 @@ The following metadata are recognized on the `<Protobuf>` items.
| GrpcOutputDir | See notes | A directory | Directory for generated gRPC stubs |
| GrpcOutputOptions | | arbitrary options | Extra options passed to gRPC codegen as `--grpc_opt=opt1,opt2` |
| GrpcServices | `both` | `none`, `client`, `server`, `both` | Generated gRPC stubs |
| AdditionalImportDirs | See notes | Directories | Additional directories for to find files |
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: "Additional directories for to find files"? Let's update to something more intelligible (also see the comment below).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

src/csharp/BUILD-INTEGRATION.md Outdated
@@ -91,6 +91,14 @@ button](https://stackoverflow.com/a/9770061). Click on it, and choose "Add as
link". If you do not select this option, Visual Studio will copy files to the
project directory instead.

Alternatively, you can also specify `AdditionalImportDirs` to give the location of proto files outside
Copy link
Contributor

Choose a reason for hiding this comment

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

rephrase "to give the location of proto files outside
the project" as e.g. "to provide the list of directories in which the protocol buffer compiler will search for imported proto files".
(your original phrasing seems a bit vague).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

jtattermusch
jtattermusch approved these changes Sep 2, 2022
View reviewed changes
Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM.

@jtattermusch
Copy link
Contributor

This PR is simply a change in docs, so test failures are unrelated.

@jtattermusch
Copy link
Contributor

I fixed a typo. I will merge once the test finish.

@jtattermusch jtattermusch merged commit dce1c83 into grpc:master Sep 2, 2022
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtattermusch jtattermusch jtattermusch approved these changes

Assignees

@jtattermusch jtattermusch

Labels
imported Specifies if the PR has been imported to the internal repository lang/C# release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tonydnewell @jtattermusch