-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
COMPONENT METADATA: translation #1934
Conversation
@@ -96,6 +96,10 @@ | |||
"type": "integer", | |||
"minimum": 1 | |||
}, | |||
"translation": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this imply we need to iterate the "version" property number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, I don't remember when it should be increased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be increased for sure if there is a breaking change - i.e. a removal. I'm not sure otherwise.
Probably doesn't need it unless we have a feature where we want users to be able to know whether something is not present vs not supported. In this case I don't think it matters or is needed but we might want to get into habit of just updating on any change - because it is hard to retrofit the ability to spot a change after this is out in the wild?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's fine with me.
@@ -0,0 +1,94 @@ | |||
{ | |||
"$id": "https://mavlink.io/translation.schema.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this URL come from? I ask because it is a 404
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It matches the file name. We don't make use of it though. In general it can be used to point to the file (e.g. online url).
@bkueng This is my last day for a bit, so if there is urgency on this we need some more people involved. The stuff here all looks good - but I'm not a JSON or schema expert, and I've almost forgotten what was agreed. So to kick this off, let me summarise what I remember, and also what am seeing. Then you can tell me if my understanding is correct. So using the example of parameters.metadata.json
That right? Re the questions for me:
What repo will all this live in? Setting up a crowdin github integration within a single repo is pretty easy - essentially you specify the source folder and a name translation format for the output files. I SHOULD be able to apply this exactly to your current repo structure. Then you could have a git action to trigger rebuilding the summary based on the translation files changing or a PR being submitted.
You can certainly do that; I guess it doesn't matter to the "system" as long as the summary contains the right information for finding any changed files for the current vehicle. You can maintain multiple translation branches in crowdin and continue to update them separately. I think that we should just maintain the main branch in crowding. So we'd update the main branch progressively and branch our repo for new releases. If we want then we can choose to manually take translations into older branches via git. Make sense? |
Thanks for the review! Didn't want to add more work to your plate before holidays.
Not at all, this has time. I was just wrapping up some longer-time outstanding things.
The URL is in component general. Otherwise you're right.
[EDIT, forgot to answer this one]: I implemented a 3 day caching policy in the GCS to avoid unnecessary frequent requests.
The repo I shared above, but moved under PX4/ when ready.
Yes that's the idea
Yes. We probably need to see how complete translations will be. |
68394f8
to
fdfe3cc
Compare
Thanks!
There is no point doing the crowdin integration until this is in the right location (or it will all have to be done again). Happy to do this when you move it. Otherwise, I hope to have mav call tonight (then away two weeks) and see if there are any further suggestions. |
@bkueng There was no particular feedback on this - it appears to match what was discussed/agreed so this can go in (20230104-Dev-Meeting) Separately there was a discussion on actuator metadata definitions and whether they are truly autopilot agnostic - at least for ArduPilot and PX4 (clearly if actuator definitions are not mapped using parameters then this won't work). My recollection is that we thought that they probably were based on the fact that both systems "just write parameters", though it is hard to be sure without an implementation. Upshot, there might need to be further iteration required on that file. |
@hamishwillee this adds the translation support as discussed in #1656.
I setup an example repo under https://github.com/bkueng/px4_metadata_translations. It explains the work-flow, CI integration is not done yet. @hamishwillee do you want to look into that? In particular for the CrowdIn integration.
TODO (outside of mavlink):