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

apply reserved keywords mapping #181

Merged
merged 3 commits into from Apr 21, 2020
Merged

apply reserved keywords mapping #181

merged 3 commits into from Apr 21, 2020

Conversation

o-mishch
Copy link
Contributor

@o-mishch o-mishch commented Apr 15, 2020

Fixes #2
This PR is ready for review.

Risk
This PR makes no API changes.

Testing Plan
Verify test app connectivity and that all unit tests pass.

Summary
avoid using keywords when generating RPC definitions.

CLA

@o-mishch
Copy link
Contributor Author

@mrapitis, @nickschwab please review this PR

@theresalech
Copy link
Collaborator

@mrapitis please let us know once you've reviewed. Then @crokita will start his review. Thanks!

@mrapitis
Copy link

@theresalech Ford has reviewed and approved this PR.

@crokita
Copy link
Contributor

crokita commented Apr 17, 2020

This is the feedback I have so far for when using the generator.

Capitalization overriding rules should only apply to the class names, and not to the enumerated values or to the methods that access them. See the following enums whose getter method names get capitalized:
- ImageFieldName
- MetadataType
- TextFieldName
- MessageType

I noticed some inconsistencies in application of the method name changes. Some also have changed the static key values to include PARAM (ex. VehicleDataResult's getResultCodeParam uses KEY_RESULT_CODE_PARAM, but in TouchEvent for getIdParam the key doesn't change. This isn't the only case).

Copy link
Contributor

@crokita crokita left a comment

Choose a reason for hiding this comment

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

Review is finished. See comment from earlier in the conversation. @o-mishch

@o-mishch
Copy link
Contributor Author

@crokita In RESERVED_KEYWORDS there are keywords KEY_RESULT_CODE and getResultCode, that’s why it was replaced appropriately in VehicleDataResult.
as well there is keyword id, but there wasn’t keyword KEY_ID, then’s why was changed only getidParam in TouchEvent

@crokita
Copy link
Contributor

crokita commented Apr 20, 2020

Thanks for the clarification. The last part of my post can be ignored, then.

@o-mishch
Copy link
Contributor Author

o-mishch commented Apr 20, 2020

@crokita issue with capitalisation of enum getters resolved in 13e04ed

@crokita
Copy link
Contributor

crokita commented Apr 21, 2020

Tested the changes and things look good. I'm going to change the base to our release branch since we have a release candidate ready for review and revisions.

@crokita crokita changed the base branch from develop to release/1.0.0 April 21, 2020 14:12
@crokita crokita merged commit 9ae9060 into smartdevicelink:release/1.0.0 Apr 21, 2020
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.

[SDL 0234] Proxy Library RPC Generation
4 participants