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

Don't use reserved words as function names in generated service #61

Merged
merged 1 commit into from
May 21, 2018

Conversation

jonbretman
Copy link
Contributor

@jonbretman jonbretman commented May 9, 2018

  • Function names in the generated service code are run through normaliseFieldObjectName to avoid invalid syntax
  • Test added to check this behaviour
  • Make sure all tests run and fix some import path errors in test/service/grpcweb.ts

@jonnyreeves
Copy link
Contributor

Thanks for raising 😀

delete is but one of many reserved keywords in JavaScript. We already have a utility which normalises field names in utils.ts (normaliseFieldObjectName) - please could you modify your patch to handle all of these keywords in a generic fashion and add some test coverage to ensure we don't regress on this change in the future?

Thanks

@jonbretman
Copy link
Contributor Author

@jonnyreeves so it looks like #56 actually stopped the tests from running 😢You can see in the Travis build that 0 test ran - https://travis-ci.org/improbable-eng/ts-protoc-gen/builds/361377789?utm_source=github_status&utm_medium=notification

> mocha --require source-map-support/register ./build/src
  0 passing (3ms)

My guess is it's this line.

I've add a look at trying to fix them but getting a failing test - #62

@jonbretman
Copy link
Contributor Author

@jonnyreeves ok all fixed now - using the normaliseFieldObjectName util function. Also only some of the tests were running so fixed that.

message StreamRequest {
string some_string = 1;
}

service SimpleService {
rpc DoUnary(UnaryRequest) returns (othercom.ExternalChildMessage) {}
rpc DoStream(StreamRequest) returns (stream othercom.ExternalChildMessage) {}

// checks that rpc methods that use reserved JS words don't generate invalid code
rpc Delete(UnaryRequest) returns (UnaryResponse) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a test to ensure methods are appropriately prefixed?

@jonnyreeves
Copy link
Contributor

Morning @jonahbron, the example protos have been moved by the change landed in #55, you will need to rebase your changes. Also, as requested, please can you add a test-case so we don't accidentally regress on this feature in the future?

Thanks :)

@jonbretman
Copy link
Contributor Author

@jonnyreeves rebased and added a test to check the name of the generated function.

Also all 91 tests are now running on this branch instead of just the 70 on master. This is because the test glob was not including the test/service/grpcweb.ts file.

@jonbretman jonbretman changed the title Dont use the name 'delete' as a function name Don't use reserved words as function names in generated service May 21, 2018
@jonnyreeves jonnyreeves merged commit ae50fbf into improbable-eng:master May 21, 2018
@jonnyreeves
Copy link
Contributor

Thanks @jonbretman; I'll look to roll out a new release of ts-protoc-gen at some point this week; hopefully we can also get the fix for #67 in as well.

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.

None yet

2 participants