Skip to content

chore: switch to generated types #1394

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

Merged
merged 10 commits into from
May 12, 2022
Merged

chore: switch to generated types #1394

merged 10 commits into from
May 12, 2022

Conversation

code-asher
Copy link
Member

@code-asher code-asher commented May 11, 2022

Closes #1210. More information can be found in the commits!

That way the renderer only takes `string` for example when rendering the
name field instead of `string | number` when the interface has some
fields that are strings and some fields are numbers.

This will be necessary when switching to generated types since some of
the fields are numbers (like the owner count on a template).
@code-asher code-asher force-pushed the asher/generated-types branch from fbf0d4c to b98aeb6 Compare May 11, 2022 19:17
In some places the organization ID is part of the URL but not part of
the request so I separated out the ID into a separate argument in the
relevant API functions.

Otherwise this was a straightforward replacement where I mostly only
needed to change some of the interface names (User instead of
UserResponse for example) and add a few missing but required properties.

Arguably `parameter_values` could be made optional but I am just passing
in an empty array for now mostly just to make it obvious that we could
pass stuff there if we need to.

I kind of winged the template form; I am not sure what the difference
between a template and template version is or why the latter comes
before the former so the form just returns all the data required to
create both.
@codecov
Copy link

codecov bot commented May 11, 2022

Codecov Report

Merging #1394 (f8f6125) into main (e8e6d3c) will increase coverage by 0.01%.
The diff coverage is 72.05%.

@@            Coverage Diff             @@
##             main    #1394      +/-   ##
==========================================
+ Coverage   67.01%   67.03%   +0.01%     
==========================================
  Files         287      288       +1     
  Lines       18850    19083     +233     
  Branches      241      241              
==========================================
+ Hits        12633    12792     +159     
- Misses       4931     4985      +54     
- Partials     1286     1306      +20     
Flag Coverage Δ
unittest-go-macos-latest 54.22% <ø> (+0.09%) ⬆️
unittest-go-postgres- 65.60% <ø> (+0.14%) ⬆️
unittest-go-ubuntu-latest 56.50% <ø> (+0.03%) ⬆️
unittest-go-windows-2022 52.60% <ø> (+0.19%) ⬆️
unittest-js 74.19% <72.05%> (-0.05%) ⬇️
Impacted Files Coverage Δ
codersdk/organizations.go 68.88% <ø> (ø)
codersdk/users.go 63.79% <ø> (ø)
codersdk/workspaces.go 63.63% <ø> (ø)
site/src/components/Footer/Footer.tsx 100.00% <ø> (ø)
site/src/components/NavbarView/NavbarView.tsx 93.33% <ø> (ø)
site/src/components/Table/Table.tsx 96.00% <ø> (ø)
site/src/components/UserDropdown/UsersDropdown.tsx 96.15% <ø> (ø)
...src/components/UserProfileCard/UserProfileCard.tsx 100.00% <ø> (ø)
site/src/components/UsersTable/UsersTable.tsx 96.00% <ø> (ø)
site/src/components/Workspace/Workspace.tsx 100.00% <ø> (ø)
... and 31 more

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 e8e6d3c...f8f6125. Read the comment docs.

Except for UserAgent which seems to be purely frontend and
ReconnectingPTYRequest which is not in codersdk so I am just leaving it
for now.
@code-asher code-asher force-pushed the asher/generated-types branch from b98aeb6 to 1de1986 Compare May 11, 2022 19:29
@code-asher code-asher marked this pull request as ready for review May 11, 2022 19:44
@code-asher code-asher requested review from presleyp and a team as code owners May 11, 2022 19:44
@greyscaled greyscaled requested a review from Kira-Pilot May 11, 2022 21:41
This reverts commit f5d1b62.

This will be added in a separate PR.
@code-asher code-asher changed the title Switch to generated types chore: switch to generated types May 11, 2022
@presleyp
Copy link
Contributor

I don't have time to properly review right now but I wanted to mention that I think more fields on CreateWorkspaceBuildRequest are optional than what shows in typesGenerated - if I understand right, everything but transition. Not sure if this is the only case missing optionality. If that's easy to fix it would be nice to have it right before we start relying more on generated types.

@code-asher
Copy link
Member Author

code-asher commented May 11, 2022

Looks like you are right. Fortunately it is pretty easy, we just need to add omitempty and then run make gen (or make site/src/api/typesGenerated.ts); I will do that now for CreateWorkspaceBuildRequest.

I am not sure what the best way to tackle this is (not sure if I have to read the code to find what is optional or if everything without a required tag can be safely considered optional) but I think I will at the very least check all the *Request interfaces to make sure they have the right optionalities.

@Kira-Pilot
Copy link
Member

nice! seems like make gen is run on save - is that right?

@code-asher
Copy link
Member Author

code-asher commented May 11, 2022

All right the request interfaces are looking good. UpdateWorkspaceAutostartRequest and UpdateWorkspaceAutostopRequest have a schedule property that could technically be made optional but I left them as-is so you have to explicitly set an empty string to disable the schedule. Not sure if that is the right call (maybe disabling should be an entirely separate endpoint but that seems like a separate discussion).

@Kira-Pilot I see make gen running on saves to .sql files (if using VS Code) but not other changes so if we make changes to interfaces or types (or if using a different editor) I think we have to run it manually (entirely possible I missed something though).

Copy link
Member

@Kira-Pilot Kira-Pilot left a comment

Choose a reason for hiding this comment

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

I'm still learning but this looks good to me!

@presleyp
Copy link
Contributor

@code-asher yay thanks! The empty string for schedule makes sense to me.

@Emyrk
Copy link
Member

Emyrk commented May 12, 2022

@Kira-Pilot we run it manually. On save is a bit excessive since it does take some time. But CI will have a failing step if you changed something and make gen needs to be run again. So I usually let CI tell me if I need to run it 🤷

Copy link
Contributor

@presleyp presleyp left a comment

Choose a reason for hiding this comment

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

Super appreciate this!

@code-asher code-asher merged commit 26b04cc into main May 12, 2022
@code-asher code-asher deleted the asher/generated-types branch May 12, 2022 15:01
@misskniss misskniss added this to the V2 Beta milestone May 15, 2022
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
* Make column renderer use the same type as its key

That way the renderer only takes `string` for example when rendering the
name field instead of `string | number` when the interface has some
fields that are strings and some fields are numbers.

This will be necessary when switching to generated types since some of
the fields are numbers (like the owner count on a template).

* Switch fully to generated types

In some places the organization ID is part of the URL but not part of
the request so I separated out the ID into a separate argument in the
relevant API functions.

Otherwise this was a straightforward replacement where I mostly only
needed to change some of the interface names (User instead of
UserResponse for example) and add a few missing but required properties.

I kind of winged the template form; I am not sure what the difference
between a template and template version is or why the latter comes
before the former so the form just returns all the data required to
create both.

* Delete handwritten types

Except for UserAgent which seems to be purely frontend and
ReconnectingPTYRequest which is not in codersdk so I am just leaving it
for now.

* Remove implemented omitempty as a future idea

This was implemented in 2d3dc43.

* Add missing optionalities to generated request interfaces
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.

Switch to using generated types
6 participants