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.

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

#23 - Add source comments from file descriptor #31

Merged
merged 7 commits into from
Feb 16, 2020

Conversation

n3rdyme
Copy link

@n3rdyme n3rdyme commented Feb 15, 2020

Added comments from fileDescriptor.sourceCodeInfo to enums, message interfaces, and services.

Should resolve issue 23
#23

Copy link
Owner

@stephenh stephenh left a comment

Choose a reason for hiding this comment

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

This looks really great!

Made a few stylistic asks/questions, but overall is great.

Can you add some comments to the simple.proto-type files that are used for ts-proto's test suite, and then in theory we should be able to see some example-generated comments in the PR (b/c the code generated for the proto/integration tests is purposefully checked in to see these sort of changes in the diffs). I could also try that if the test suite setup is too jank (it's not just npm test), but their is an overview in the readme that hopefully works. Let me know if it doesn't / you don't have time, and I'll throw in a few example comments to see what the output looks like before merging.

src/sourceInfo.ts Outdated Show resolved Hide resolved
src/sourceInfo.ts Outdated Show resolved Hide resolved
src/sourceInfo.ts Outdated Show resolved Hide resolved
src/sourceInfo.ts Outdated Show resolved Hide resolved
@n3rdyme
Copy link
Author

n3rdyme commented Feb 16, 2020

@stephenh
I added the requested comments in simple.proto and cleaned up the conditional logic a bit. I assume you did not want me to regenerate all the binary and source files on branch. It seems like that might make merging difficult, so I assume you are keeping PR branches clean?

Otherwise, I think I covered all the asks above. Let me know if you need any additional changes.

@stephenh
Copy link
Owner

Looks great, thanks! I do actually check in the generated source files; I know in downstream / real projects that is an anti-pattern but given ts-proto's job is producing that output, I basically treat it as an mini-integration test to see how the output has changed in the PRs/diffs/etc. I'll go ahead and do a follow up commit with those changes now.

@stephenh stephenh merged commit 09d024e into stephenh:master Feb 16, 2020
zfy0701 added a commit to sentioxyz/ts-proto that referenced this pull request Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants