search

ftrackhq / ftrack-ts-schema-generator

Gets the schema from an ftrack instance and generates typescript interfaces for all the entity types in the instance.
Apache License 2.0
2 stars 1 forks source link

feat: introduce tests #23

Closed ffMathy closed 11 months ago

ffMathy commented 11 months ago

Changes

Test

ffMathy commented 11 months ago

Hey @gismya here's my plan for this PR and upcoming ones.

  1. Introducing tests.
  2. I'll be switching to TS-SDK for emitting (fixing #8).
  3. I'll be introducing support for types and object types, and having a list of those available at runtime.

This is the first PR. In that regard, I have some questions:

  1. Is it correct you want to use vitest for tests still?
  2. Can you elaborate on why QuerySchemasResponse[] is used? So in essence, there's an array of arrays of schemas? What does this structure mean?
gismya commented 11 months ago

Hey @gismya here's my plan for this PR and upcoming ones.

  1. Introducing tests.
  2. I'll be switching to TS-SDK for emitting (fixing feat: use TypeScript's emit SDK #8).
  3. I'll be introducing support for types and object types, and having a list of those available at runtime.

Tests are always good. With only a cursory look, utilizing the TS-SDK seems a bit more complicated to use and maintain than just string concatenation. What are the main benefits of doing it? I wouldn't want you spending time and effort on something we later find too complex to merge or something like that. The complicated of the current code, in my mind, is the handling of the schemas, not the actual creation of the TS file?

Could you elaborate on point 3?

This is the first PR. In that regard, I have some questions:

  1. Is it correct you want to use vitest for tests still?
  2. Can you elaborate on why QuerySchemasResponse[] is used? So in essence, there's an array of arrays of schemas? What does this structure mean?

Yes, vitest is the preferred testing solution. QuerySchemasResponse[] is because that's what's returned from the session call in getSchemas, since session.call can take multiple calls at the same time. Then that first layer of array should have been flattened, but I missed that when writing the initial implementation and didn't really see it until you asked now :)

ffMathy commented 11 months ago

Thanks for elaborating! 😍

As for the tssdk, I wouldn't be worried. Get what you mean, but I don't think it'll be too bad. And I'll make it as a separate PR afterwards 😊🙏

ffMathy commented 11 months ago

As for the benefits of it, the main benefit is higher readability 😊

ffMathy commented 11 months ago

As for point 3, I'd love if the object types and types in ftrack were emitted as a string union type.

ffMathy commented 11 months ago

This is now ready for review! I ended up going for snapshot testing in vitest. I think this is a good match for types of tests like these.

Not sure if you've used it before, but if a snapshot fails, it shows a diff, and then provides you with an option to accept the new shapshot.

That way, it becomes clear what effects a change had. Works well with watch mode too.

gismya commented 11 months ago

As for the benefits of it, the main benefit is higher readability 😊

If you manage to make it more readable I'm all for it. But I think the parsing part is the confusing part, not the actual writing to the schema. But I'd love to be wrong :)

ffMathy commented 11 months ago

As for the benefits of it, the main benefit is higher readability 😊

If you manage to make it more readable I'm all for it. But I think the parsing part is the confusing part, not the actual writing to the schema. But I'd love to be wrong :)

Yeah on that front I am also hoping for improvements.

But either way, I know it's opinionated and risky, and there'll be no hard feelings if the PR is rejected. I'm having fun working on it either way.

What do you think about the PR from a review perspective?

gismya commented 11 months ago

What do you think about the PR from a review perspective?

I don't know. :) I'll try to make time for reviewing it later today.

ffMathy commented 11 months ago

Thanks! I corrected everything now.

Do we merge now, or do we wait until that Ftrack schema bug is fixed, so the tests pass?

gismya commented 11 months ago

You can merge now

ffMathy commented 11 months ago

Cool! Do I have permissions to merge it? Doesn't look like it 🫣

gismya commented 11 months ago

Right, forgot about that detail