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

feat(typegen): retain intersection types #389

Merged
merged 11 commits into from
Apr 16, 2022
Merged

Conversation

janpaepke
Copy link
Collaborator

This PR is based on the discussion here:
#386

@janpaepke
Copy link
Collaborator Author

So this works surprisingly well. So much so, that I don't fully trust I didn't overlook something.
I would ask you to test it please.

Additionally I restructured the readme quite a bit.

  • I removed the redundancy of the config and cli param definitions by combining them into a table.
  • added docs for this feature.

Looking through the readme now, I feel like everything that's in "Advanced Configuration" should maybe go into a separate file. What do you think?

@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2022

Codecov Report

Merging #389 (e4b25f2) into master (32acf69) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #389      +/-   ##
==========================================
+ Coverage   96.69%   96.72%   +0.03%     
==========================================
  Files          32       32              
  Lines        1119     1130      +11     
  Branches      214      216       +2     
==========================================
+ Hits         1082     1093      +11     
  Misses         37       37              
Impacted Files Coverage Δ
packages/typegen/src/write/inline.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32acf69...e4b25f2. Read the comment docs.

Copy link
Owner

@mmkal mmkal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! I think we’re gonna immediately start using the to get rid of loads of bogus nulls. Will take a closer look at the docs and details over the next day or two. Let me know if you’re still working on this - one thing that occurs to me is it’d be good to have a slightly more high level test as well. i.e. no templated params, so it’s easy to see at a glance how this feature works. I’d be happy to push this to your branch if you’re not actively reshuffling stuff - let me know.

@mmkal
Copy link
Owner

mmkal commented Apr 14, 2022

So this works surprisingly well. So much so, that I don't fully trust I didn't overlook something. I would ask you to test it please.

Additionally I restructured the readme quite a bit.

  • I removed the redundancy of the config and cli param definitions by combining them into a table.
  • added docs for this feature.

Looking through the readme now, I feel like everything that's in "Advanced Configuration" should maybe go into a separate file. What do you think?

I like the readme updates - I am generally not a fan of splitting readme files unless there’s a full docusaurus site because it loses cmd-f without an adequate replacement. Also just to make the diff cleaner, what do you think about pushing in the unrelated readme changes first so it’s easy to look back at this PR and see what was added?

@janpaepke
Copy link
Collaborator Author

janpaepke commented Apr 15, 2022

Hey Misha,
sure, I apologise for falling into my bad habbit of "while I'm at this, I might as well do that".
You're right, it should have been a separate PR, even though the changes in the readme are in distinct sections, there is no overlap. Look here for the config only changes and here to preview the new readme.

The docs for the new feature are here.

Re tests: sure, have a go! Don't worry, I can keep my hands busy elsewhere. ;)

@janpaepke janpaepke force-pushed the feature/type-enhancements branch from 2c857b9 to fc68043 Compare April 15, 2022 09:53
@janpaepke
Copy link
Collaborator Author

janpaepke commented Apr 15, 2022

Damn it, I found a caveat.
Intersecting the types will drop the comments.
So basically you lose the info what column the property comes from, which typegen graciously tells you.

Off the top of my head I don't have a solution. Do you?

edit: forget what I said. I was being stupid.

packages/typegen/src/write/inline.ts Show resolved Hide resolved
packages/typegen/src/write/inline.ts Outdated Show resolved Hide resolved
packages/typegen/readme.md Outdated Show resolved Hide resolved

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@janpaepke
Copy link
Collaborator Author

Sooo. Is this good to go?

@janpaepke janpaepke changed the title Draft: feat(typegen): retain intersection types feat(typegen): retain intersection types Apr 16, 2022
Copy link
Owner

@mmkal mmkal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@mmkal mmkal merged commit 81ab1fa into master Apr 16, 2022
@mmkal mmkal deleted the feature/type-enhancements branch April 16, 2022 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants