foxglove / schemas

Message schemas supported by Foxglove Studio
MIT License
55 stars 29 forks source link

Add Time type to exports for typescript types #131

Closed rachwalk closed 5 months ago

rachwalk commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/foxglove/schemas/blob/dcc3b66c69506258baf46baf08e99791340e33bb/internal/exportTypeScriptSchemas.ts#L34-L37

rachwalk commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

@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 commented 5 months ago

Closing for no response.