cql-execution icon indicating copy to clipboard operation
cql-execution copied to clipboard

Explore elm types

Open csenn opened this issue 2 years ago • 5 comments

This Pull Request starts the addition of ELM Typescript Types that can be used to make the ELM traversal logic a little more type safe.

Pull requests into cql-execution require the following. Submitter and reviewer should ✔ when done. For items that are not-applicable, mark "N/A" and ✔.

CDS Connect and Bonnie are the main users of this repository. It is strongly recommended to include a person from each of those projects as a reviewer.

Submitter:

  • [ ✔] This pull request describes why these changes were made
  • [ ] Code diff has been done and been reviewed (it does not contain: additional white space, not applicable code changes, debug statements, etc.)
  • [ ] Tests are included and test edge cases
  • [ ] Tests have been run locally and pass
  • [ ] Code coverage has not gone down and all code touched or added is covered.
  • [ ] Code passes lint and prettier (hint: use npm run test:plus to run tests, lint, and prettier)
  • [ ] All dependent libraries are appropriately updated or have a corresponding PR related to this change
  • [ ] cql4browsers.js built with npm run build:browserify if source changed.

Reviewer:

Name:

  • [ ] Code is maintainable and reusable, reuses existing code and infrastructure where appropriate, and accomplishes the task’s purpose
  • [ ] The tests appropriately test the new code, including edge cases
  • [ ] You have tried to break the code

csenn avatar Mar 14 '22 18:03 csenn

Sorry I let this linger for so long... I've responded in some of the conversations with my thoughts. Interested to hear yours,

cmoesel avatar May 13 '22 16:05 cmoesel

@csenn - I've given this another round of review. I think there are a few small tweaks that would be good to make, but overall everything looks to be in good shape. The question on whether or not to throw in the constructor is still unresolved. I think it's probably OK to keep it as you have it in this PR, but I've also asked a colleague for additional input before making the call.

So... I think this branch needs to be brought up to date w/ master and a few very minor things (mainly doc/ts-ignore) need to be tweaked. I realize that this PR has been open for some time now, so if you (@csenn) don't have time to revisit it, I could potentially make the changes myself if that is OK w/ you. Let me know how you want to proceed.

cmoesel avatar Oct 03 '22 16:10 cmoesel

Agreed with @cmoesel on the constructor throwing. I think it's okay as is.

One thing I wanted to ask about was the intention of these ELM types going forward. I think we're happy to include the elm.d.ts in the cql-execution project, but were there hopes of getting this into DefinitelyTyped so we can use a @types/elm in any TypeScript project? Not entirely sure what the process of that would be, but safe to say I think we'd be happy to include the type declarations in the source code of cql-execution in the meantime

mgramigna avatar Oct 03 '22 17:10 mgramigna

Hey @cmoesel and @mgramigna, sorry this fell on the wayside. I used the ELM types generated in my own projects, and it's been very helpful for my needs, but the question is are the types good enough for DefinitleyTyped. Here is the repo I used to generate the types: https://github.com/cqlabio/cql-elm-types.

It converts the XSD into Typescript types. Unfortunately, initially i tried to keep it simple and just use libraries that converted XSD into json_schema and json_schema into typescript types, however the types did not look good. So I ended up just writing some code that converts it manually, but it's not super well tested.

So either A) we can try and validate and possibly improve what I built or B) we can just create a new de-facto repo with the index.d.ts file and basically hand code changes for now on. What do you guys think? Does ELM change enough that automated would be better? Is that XSD reliable enough or does hand crafting have some advantages (such as marking something required that is missing in the XSD)?

In terms of this PR, it would probably be better to just restart it since it only took 0.5 hours - 1 hour if I remember right. Is there a plan for any large architecture changes to what's currently on master/main?

csenn avatar Oct 04 '22 21:10 csenn

@csenn -- I think it makes sense to target DefinitelyTyped. I wouldn't worry too much about some types not being 100% perfect due to idiosyncrasies, etc; it's easy enough to fix them if we find anything like that.

As for automating type generation vs hand-maintaining from here forward... I feel like ELM is pretty stable at this point. If getting the automation solid requires a lot of hacks/workarounds or is just generally difficult, I feel like hand-maintaining the types should be quite feasible. It also allows for some one-offs and judgement calls that might not be possible in automation.

All that said, before we decide how to move forward, I think it would be good to bring @brynrhodes into this conversation. Have you talked to him about this at all? @bkaney is also a big typescript fan who does some work in CQL, so he might have some feedback and tips as well.

cmoesel avatar Oct 06 '22 14:10 cmoesel