Fix Typescript generation to work with node-mavlink #778
Conversation
GimpMaster
changed the title
|
Just want to express my support for this contribution, as the current typescript generated library does not even work anymore. Me and my team would love to have this PR merged! |
generator/mavgen_typescript.py
Outdated
| @@ -74,40 +76,55 @@ def generate_classes(dir, registry, msgs, xml): | |||
|
|
|||
| f.write("/*\n{}\n*/\n".format(m.description.strip())) | |||
| for field in m.fields: | |||
| f.write("// {} {} {}\n".format(field.name, field.description.strip(), field.type)) | |||
| f.write("/* {} {} {} */\n".format(field.name, field.description.strip(), field.type)) | |||
There was a problem hiding this comment.
Fields comments should be moved at line 85/86, otherwise they won't show up
There was a problem hiding this comment.
I'm not sure I understand this comment?When I look at lines 84/85 I don't see any comments. What are you proposing?
There was a problem hiding this comment.
Just to move the comments from here:
/*
TC ping (expects an ACK message as a response)
*/
/* timestamp Timestamp to identify when it was sent uint64_t */
export class PingTc extends MavLinkData {
public timestamp!: number;
...to here:
/*
TC ping (expects an ACK message as a response)
*/
export class PingTc extends MavLinkData {
/* Timestamp to identify when it was sent uint64_t */
public timestamp!: number;
...Also notice how I removed the field name from the comment, as it is not really necessary.
|
Hello, is there any repo maintainer willing to take the time to look at this PR? It's kind of a big deal for me to have it merged in the repo... Maybe @tridge ? (sorry for the mention but you are the only active repo admin I was able to find, as there's no list of collaborators) |
|
@davidbuzz I think you are JS/TS guy, any opinion on this ? |
…fields - Added in helper utility function to create list of message ids to magic numbers for use with custom messaging in node-mavlink
# Conflicts: # generator/mavgen_typescript.py
|
Based upon feedback from this node-mavlink issue disscussion (ArduPilot/node-mavlink#21) we have this working correctly with custom messages now. I've updated the PR description. |
|
Hello, any news? |
|
@davidbuzz Are you able to approve? |
|
The changes this pr makes in the generator/javascript/ folder should NOT be merged... thats a different generator['javascript' vs 'typescript'], please remove them from this pr. As for the typescript changes, i cant comment, as i maintain the other one. ;-) |
|
Thanks @davidbuzz - I'm not sure how those got in there, but I will revert those two file changes. Do you know who can review the typescript changes? |
|
I dont know if we have a maintainer for 'typescript' bindings, but 'git blame' them to see whos made changes b4, or who created them in the first place... or failing that, convince the rest of us with testing evidence that its better.. ;-) |
Ping @pascalgross - from 2019... if we don't get a response we'll probably just welcome you as the new typescript generator maintainer :-) |
While recently testing the typescript generation with the latest node-mavlink we found a number of issues. For example
This is now working with the current head of node-mavlink for us.
NOTE
Added in a helper utility method for generating a list of all messages for use with node-mavlink custom message support. This new exported function is generated in message-registry.ts
Also adjusted based on comments.