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

Add Time type to exports for typescript types #131

Closed
wants to merge 3 commits into from

Conversation

rachwalk
Copy link

@rachwalk rachwalk commented Oct 5, 2023

Public-Facing Changes

Adds export of the Time type for typescript type

Description

As far as I can see there is no reason for the Time type not to be exported. Exporting it is a quality of development improvement when writing plugins relying on timestamps used in the other exported types.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jtbandes
Copy link
Member

jtbandes commented Oct 5, 2023

Thanks for the PR. I think it makes sense to add Time & Duration to the exports. However, this index.ts is a generated file, so you'd need to update the code that generates it here:

let indexTS = "";
for (const schema of allSchemaNames) {
indexTS += `export * from "./${schema.name}";\n`;
}

@rachwalk
Copy link
Author

rachwalk commented Oct 6, 2023

Thanks @jtbandes for pointing that out, I've started working on the autogeneration as you've reccomended. I have a question though, what is the reason for considering the time and duration types, as FoxglovePrimitive? In the entire code base they require special handling, and it is difficult to consider a type with two fields primitive.

Wouldn't it make more sense to create another type for the two? In addition to FoxglovePrimitive, FoxgloveMessage, and FoxgloveEnum, something like FoxgloveTime? It seems it would allow to make the code more consistent and allow to not have primitives which are themselves composed out of other primitives.

@jtbandes
Copy link
Member

Treating these as primitives was inherited from the ROS 1 msg format where time and duration are primitives, and when we translate our types to ROS 1 .msg we need to keep them as time and duration. The specific way that they are represented in this codebase with type: "primitive" could probably be improved, but it's also sort of immaterial since it's mostly an internal representation that's used while generating the output schemas.

@defunctzombie
Copy link
Contributor

defunctzombie commented Jan 23, 2024

@rachwalk Could you fix up the CI build error and we can take another look. Sorry for the delay on this. You'll also need to sign the CLA for us to merge this.

@defunctzombie
Copy link
Contributor

Closing for no response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants