Skip to content

feat: add info comments to type generator - #291

Merged
nsalamad merged 1 commit into
mainfrom
DVC-8835-types-generator-with-comments
Sep 15, 2023
Merged

feat: add info comments to type generator#291
nsalamad merged 1 commit into
mainfrom
DVC-8835-types-generator-with-comments

Conversation

@nsalamad

@nsalamadnsalamad commented Sep 14, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor
image

@nsalamad
nsalamad requested a review from a teamSeptember 14, 2023 14:07
@nsalamad
nsalamadforce-pushed the DVC-8835-types-generator-with-comments branch 3 times, most recently from 13adbd6 to 55dadbbCompareSeptember 14, 2023 20:44
elliotCamblor
elliotCamblor reviewed Sep 15, 2023
View reviewed changes

@elliotCamblorelliotCamblor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some (mostly) nitpicky comments

Comment threadsrc/commands/generate/types.ts Outdated
Comment threadsrc/commands/generate/types.ts
}

private async getTypeDefinitionLine(variable: Variable, orgMembers: OrganizationMember[]): Promise<string> {
const { flags } = await this.parse(GenerateTypes)

@elliotCamblorelliotCamblorSep 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it make sense to just pass the flags around this these functions rather than having to parse them out 2-3 times?

@nsalamadnsalamadSep 15, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I personally don't like the idea of passing the args around cause it clutters the definitions/calls and makes it more likely that someone will mistakenly pass the args in the wrong positions. Like some of the methods are gonna end up having lots of args i.e. getTypesString with 6 which is too much imo

@nsalamadnsalamadSep 15, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

was even considering changing getTypesString to parse as well

Comment threadsrc/commands/generate/types.ts Outdated
@nsalamad
nsalamadforce-pushed the DVC-8835-types-generator-with-comments branch from 55dadbb to 35c37b1CompareSeptember 15, 2023 15:37
@nsalamad
nsalamadforce-pushed the DVC-8835-types-generator-with-comments branch from 35c37b1 to a916809CompareSeptember 15, 2023 15:37
@nsalamad
nsalamad merged commit 3f83f79 into mainSep 15, 2023
@nsalamad
nsalamad deleted the DVC-8835-types-generator-with-comments branch September 15, 2023 16:59
@jonathannorrisjonathannorris mentioned this pull request Aug 5, 2026
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@elliotCamblorelliotCamblorelliotCamblor approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

5 min review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nsalamad@elliotCamblor