#2 [SDL-0234] Proxy Library RPC Generation #105

vladmu · 2020-01-14T12:39:48Z

Fixes #2

This PR is [ready] for review.

Risk

This PR makes [no] API changes.

Testing Plan

Covered by unit tests and PyLint

Summary

According to [SDL-0234] Proxy Library RPC Generation proposal, this PR adds the Python generator script based on XML Parser from smartdevicelink/rpc_spec linked to the repository as Git submodule

Changelog

Enhancements

Added lib/rpc_spec submodule pointed to master branch of smartdevicelink/rpc_spec repository
Created Proxy RPC Generator based on Python 3.5 and linked with parser from the lib/rpc_spec submodule
Added README.md with usage description and transformation rules for Proxy RPC Generator

CLA

I have signed the CLA and this pull request adheres to the Contributing Guide.

* Added `lib/rpc_spec` submodule pointed to `master` branch of `smartdevicelink/rpc_spec` repository * Created `Proxy RPC Generator` based on Python 3.5 and linked with parser from the `lib/rpc_spec` submodule * Added `README.md` with usage description and transformation rules for Proxy RPC Generator

…poses

TinaKleczka · 2020-01-16T16:01:45Z

@theresalech please review

joeygrover · 2020-01-16T16:41:45Z

.gitmodules

@@ -0,0 +1,4 @@
+[submodule "lib/rpc_spec"]
+	path = lib/rpc_spec
+	url = git@github.com:vladmu/rpc_spec.git


As a reminder, this was agreed upon and is only temporary. The PR should not be merged until this is updated to reflect the correct remote. The rest of this PR can be reviewed as normal.

changed to:

[submodule "lib/rpc_spec"] path = lib/rpc_spec url = https://github.com/smartdevicelink/rpc_spec.git branch = develop

crokita · 2020-01-17T15:21:53Z

We would appreciate if developers did not force push to a PR that is in review. Force pushing makes it difficult to review a PR because we cannot tell what was changed since the last commit. In general, please do not rebase and do not force push against the SDL repositories. Instead, you can merge the current develop branch into your PR branch. Thank you.

vladmu · 2020-01-17T15:37:42Z

you can merge the current develop branch into your PR branch

@crokita got it, thank you for advice. We will do merge instead of rebase for refreshing the branch next time.

crokita

ESLint overrides should be added to parts of the files specifically for camelcase naming violations from RPC spec data. See here: https://eslint.org/docs/user-guide/configuring.html#disabling-rules-with-inline-comments
All other lint checks should pass. Our linter rules are in the .eslintrc file.

The example apps break as a result of the generator's changes. Therefore, the affected parts of the library and the example apps will need to be fixed before merging this PR. We can make those fixes ourselves.

@nickschwab can also answer questions regarding the feedback.

crokita · 2020-01-17T19:24:21Z

generator/README.md

+The class should have the next JSDoc comment:
+```javascript
+/**
+ * [decription]


change to "description"

crokita · 2020-01-17T19:24:29Z

generator/README.md

+The getter should have the next JSDoc comment:
+```javascript
+/**
+ * [decription]


change to "description"

crokita · 2020-01-17T19:24:36Z

generator/README.md

+The class should have the next JSDoc comment:
+```javascript
+/**
+ * [decription]


change to "description"

crokita · 2020-01-17T19:24:41Z

generator/README.md

+The setter should have the next JSDoc comment:
+```javascript
+/**
+ * @param {[param_type]} [value_name] [decription]


change to "description"

crokita · 2020-01-17T19:24:49Z

generator/README.md

+The class should have the next JSDoc comment:
+```javascript
+/**
+ * [decription]


change to "description"

crokita · 2020-01-17T19:24:54Z

generator/README.md

+The setter should have the next JSDoc comment:
+```javascript
+/**
+ * @param {[param_type]} [value_name] [decription]


change to "description"

crokita · 2020-01-17T19:58:45Z

generator/mapping.json

+{
+  "enums": {
+    "DisplayType": {
+      "params_additional": [


The TESTING enum was mistakenly added to the DisplayType enums. This can be removed.

crokita · 2020-01-17T21:15:31Z

generator/mapping.json

@@ -0,0 +1,140 @@
+{


There isn't documentation for the effect of each key-value pair that appears in this file, ex. params_additional and the array value it has. This would be useful information to add to the README of how each override rule works, as we may need to make changes to this file in future releases.

There should be a clearer distinction as to whether properties under a struct/enum/message are attributes of that class, or whether they are doing some modification to that class. Ex. under RegisterAppInterfaceRequest there is appID and fullAppID which are properties of RegisterAppInterfaceRequest, but there is also the params_additional property on the same level which wouldn't actually be a property of RegisterAppInterfaceRequest. Another example is PutFileRequest, where syncFileName is a property of the class, but script is not and it performs another effect.

The documentation added into readme b30da93

crokita · 2020-01-17T21:21:15Z

generator/mapping.json

+      },
+      "script": "templates/scripts/PutFileRequest.js"
+    },
+    "OnHMIStatus": {


This object can be removed. We will keep the naming convention from the spec as OnHMIStatus.

BorisTm · 2020-01-27T09:59:27Z

ESLint overrides should be added to parts of the files specifically for camelcase naming violations from RPC spec data. See here: https://eslint.org/docs/user-guide/configuring.html#disabling-rules-with-inline-comments
All other lint checks should pass. Our linter rules are in the .eslintrc file.

@crokita Question about camelcase naming violations. Do we need to add these 30 parameters as exceptions to .eslintrc? Or is it better to add processing to the parser for such cases?

We have now camelcase naming violations for these parameters/functions:
struct:
DateTime.js
setTz_hour
tz_hour
getTz_hour

setTz_minute
tz_minute (set, get, parameter)
getTz_minute

enum:
Dimension.js
Dimension_NO_FIX
Dimension_2D
Dimension_3D

messages:
GetVehicleData.js
setFuelLevel_State
level_state
getFuelLevel_State

GetVehicleDataResponse.js
setFuelLevel_State
level_state
getFuelLevel_State

OnVehicleData.js
setFuelLevel_State
level_state
getFuelLevel_State

SubscribeVehicleData.js
setFuelLevel_State
level_state
getFuelLevel_State

unsubscribeVehicleData.js
setFuelLevel_State
level_state
getFuelLevel_State

SubscribeVehicleDataResponce.js
setFuelLevel_State
level_state
getFuelLevel_State

UnsubscribeVehicleDataResponce.js
setFuelLevel_State
level_state
getFuelLevel_State

nickschwab · 2020-01-27T14:51:18Z

@BorisTm Thanks for asking! Instead of placing the overrides in the .eslintrc file, please use in-line override comments in the generated files themselves: https://eslint.org/docs/user-guide/configuring.html#disabling-rules-with-inline-comments

e.g.

// eslint-disable-next-line camelcase
[line which violates the camelcase rule here]

vladmu · 2020-01-27T15:01:09Z

@nickschwab thank you for advice, from the perspective of the manual changes it is the best approach, but it seems this requires additional code for auto-checking names on the fly during the generation process and mark invalid lines with the corresponding comment, which could be slightly complicated than the adjusting of .eslintrc at the current stage. Is that really required?

nickschwab · 2020-01-27T15:05:52Z

@vladmu I recommend either applying the in-line override to all entities (rather than performing a check to determine whether or not each entity actually violates the rule), or we can use a file-scoped override to specifically ignore camelcase violations for the entire file (e.g. /* eslint-disable camelcase */ at the top of the file)

vladmu · 2020-01-27T15:08:10Z

@nickschwab the last recommendation is simplest, but in this case, all RPC names will not be validated, is that suitable?

nickschwab · 2020-01-27T15:26:20Z

@vladmu Yes 👍

crokita · 2020-01-27T20:00:22Z

Hello! Thank you for making some of the requested revisions. After a more thorough review of the code we only have a few additional changes to suggest before approval.

The validateType method in RpcStruct accepts a third argument (isArray) which makes it capable of validating arrays of objects. This argument should be used instead of ignoring array types
Regarding JSDoc comments with describing arrays of objects, it needs to be either the format Array.<Object> or Object[] instead of currently Array<Object>. We suggest using Object[].
The variables used in for loops for the template scripts shouldn't have single character identifiers (ex. looping over 'methods', the iterating variable should be more descriptive like 'method'). This makes it easier for us to understand the code when looking through it.
Per the guidelines regarding Enum generation: The value of the pair shall be set to the value of the hexvalue attribute (if available), otherwise the name attribute. Currently the only instance is with the FunctionID enum class (ex. RegisterAppInterface. Value is 1 instead of 0x01)
RpcCreator.js is a class that needs to be generated alongside the other RPC files. We have a currently unfinished version at lib/js/src/rpc/RpcCreator.js that will need to have its case statements and imports expanded to include all RPCs.

theresalech · 2020-01-29T21:24:19Z

@vladmu please let us know once all feedback has been addressed and this PR is ready for Livio's re-review. Thanks!

vladmu · 2020-01-30T13:52:01Z

@theresalech We've done all the requested changes. So this PR is now ready for Livio's review again. Thank you!

crokita · 2020-01-30T18:31:27Z

@vladmu
Thank you again for the changes! I've done another review and have the following revisions:

Below are change requests to the mappings.json file:

enums.FunctionID object can be removed
structs.DisplayCapabilities object can be removed
structs.TextField can be removed
structs.RGBColor can be removed
functions.OnHMIStatusNotification can be removed
functions.RegisterAppInterfaceResponse can be removed

Here are other notes:

I think scripts broke for the mapping.json file. I don't see PutFileRequest or RegisterAppInterfaceRequest custom code in the generated output
method_title is the property used in the mapping.json, but in the README file it says method_name
ESLint tests all pass except for the rule "id-length", for files Rectangle.js, TouchCoord.js, and TouchEvent.js. I think having another override which doesn't report those warnings should be made, just like how the camelcase rule is ignored for these files.

vladmu · 2020-01-31T14:31:36Z

@crokita thank you, all points are fixed. Please review.

crokita · 2020-01-31T16:07:51Z

@vladmu Looks good 👍
I'll be doing some final checks and include the generated output for the RPCs in this PR

vladmu · 2020-01-31T17:38:27Z

.gitmodules

@@ -1,4 +1,4 @@
 [submodule "lib/rpc_spec"]
 	path = lib/rpc_spec
-	url = git@github.com:vladmu/rpc_spec.git
+	url = https://github.com/smartdevicelink/rpc_spec.git


@crokita you pointed the submodule to the correct repository, but please take into account the master branch of rpc_spec doesn't contain the parser code yet. The corresponding PR is not merged yet. It would be good if you notify @JackLivio in answer to this comment that JS generator passed your checks and rpc_spec could be merged too.

vladmu mentioned this pull request Jan 14, 2020

#203 SDL-0234 Proxy Library RPC Generation smartdevicelink/rpc_spec#202

Merged

1 task

Pointed lib/rpc_spec submodule to the personal fork for testing pur…

aa30209

…poses

vladmu requested review from theresalech and Jack-Byrne January 16, 2020 16:07

theresalech requested review from crokita and nickschwab and removed request for Jack-Byrne January 16, 2020 16:37

joeygrover reviewed Jan 16, 2020

View reviewed changes

refactoring according to comments in pull/202

2e90da7

crokita reviewed Jan 17, 2020

View reviewed changes

o-mishch and others added 4 commits January 21, 2020 15:36

align changes from parser rpc_spec

604bc1b

align with rpc_spec

8d452a6

minor chnages after code review

a227a48

Fixed typo 'decription'->'description'

3d33e2a

improve manual mapping

325bc23

vladmu and others added 4 commits January 28, 2020 13:40

Added Custom mapping section into README.md

b30da93

Minor fix of customization spec

34ef524

adding RpcCreator processing

e05d423

adding RpcCreator.js: year in Copyright

6821ddf

o-mishch added 3 commits January 29, 2020 16:11

updating auto-tests

f3f5656

Trailing spaces not allowed in RpcCreator

904a7c5

resolving issue with GenericResponseResponce

dd8291c

handle exeption in xsd

e0eac24

o-mishch added 2 commits January 31, 2020 10:54

applying changes requested in review

ae6dd2b

updating .eslintrc

59df96a

crokita added 2 commits January 31, 2020 11:24

Add generated files and update the library and example apps

cb10518

Resolve merge conflicts

f03db6d

crokita merged commit d89dcd4 into smartdevicelink:develop Jan 31, 2020

vladmu commented Jan 31, 2020

View reviewed changes

vladmu deleted the feature/#2-SDL-0234-Proxy-Library-RPC-Generation branch February 3, 2020 11:02

#2 [SDL-0234] Proxy Library RPC Generation #105

#2 [SDL-0234] Proxy Library RPC Generation #105

Conversation

vladmu commented Jan 14, 2020

Risk

Testing Plan

Summary

Changelog

Enhancements

CLA

TinaKleczka commented Jan 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crokita commented Jan 17, 2020

vladmu commented Jan 17, 2020

crokita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BorisTm commented Jan 27, 2020

nickschwab commented Jan 27, 2020

vladmu commented Jan 27, 2020

nickschwab commented Jan 27, 2020

vladmu commented Jan 27, 2020

nickschwab commented Jan 27, 2020

crokita commented Jan 27, 2020

theresalech commented Jan 29, 2020

vladmu commented Jan 30, 2020

crokita commented Jan 30, 2020

vladmu commented Jan 31, 2020

crokita commented Jan 31, 2020

Choose a reason for hiding this comment