improve type extraction #399
Conversation
janpaepke
commented
Jun 10, 2022
•
janpaepke
commented
- refactor getColumnInfo to separate responsibilities
- make functions idempotent
- decrease repetition
- improve field analysis
Codecov Report
@@ Coverage Diff @@
## master #399 +/- ##
==========================================
+ Coverage 96.78% 96.79% +0.01%
==========================================
Files 32 33 +1
Lines 1152 1156 +4
Branches 218 218
==========================================
+ Hits 1115 1119 +4
Misses 33 33
Partials 4 4
Continue to review full report at Codecov.
|
|
I decided to do this in steps, as the changes are likely going to be significant. This is already good to review, I'll create the next pr towards this branch for individual review. |
| const tagOptions = (query: DescribedQuery) => { | ||
| const buildGetFieldInfo = (viewResult: ViewResult[], ast: SelectFromStatement) => { | ||
| const viewableAst = | ||
| viewResult[0]?.formatted_query === undefined ? ast : getHopefullyViewableAST(viewResult[0].formatted_query!) // TODO: explore why this fallback might be needed - can't we always use the original ast? |
There was a problem hiding this comment.
Similar to last time, I’d prefer a truthiness check than an explicit comparison to undefined
|
Correct. They're notes for the subsequent ones :) This is gonna take a while :) |
|
@janpaepke let's merge this! Any reason to keep it as draft for now? A minor thing I want to add now typegen-ignore is in is (optionally) throwing when some queries couldn't be typed. But I don't want to add that on top of current main and cause conflicts. |
|
Hey misha, well this is really only half way there and has also changed quite a bit in my local version. We could merge this currently still only cosmetic. |
|
@janpaepke let's merge. I'd like to make some changes too, and want to reduce the chance of conflicts. Plus, cosmetic changes are still good changes! If there are small tweaks you want to make, feel free then mark ready for review. Or, if it's going to cause issues for your local somehow, let me know and I can hold off/we can figure out how to separate out this from future changes. Otherwise will just merge it at some point in the next few days. |
|
Nah, it's good to merge. |
