Skip to content

feat(codegen): Add WithResolverTransformer option#164

Merged
candiduslynx merged 2 commits intomainfrom
feat/custom-resolver-for-type
Sep 22, 2022
Merged

feat(codegen): Add WithResolverTransformer option#164
candiduslynx merged 2 commits intomainfrom
feat/custom-resolver-for-type

Conversation

@candiduslynx
Copy link
Copy Markdown
Contributor

@candiduslynx candiduslynx commented Sep 21, 2022 •
edited
Loading

Add WithResolverTransformer option (based on #150)

@candiduslynx candiduslynx changed the title feat(codegen): Add WithTypeResolver option (based on #150) feat(codegen): Add WithTypeResolver option Sep 21, 2022
@github-actions github-actions bot added feat and removed feat labels Sep 21, 2022
@candiduslynx candiduslynx self-assigned this Sep 21, 2022
@github-actions github-actions bot added feat and removed feat labels Sep 21, 2022
hermanschaaf
hermanschaaf reviewed Sep 21, 2022
View reviewed changes
yevgenypats
yevgenypats suggested changes Sep 21, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Same request :)

Very hard to review major breaking features for public APIs with refactor code that moves stuff around.

Once PR is only with a given change, I'd love also to see a real world example of usage with the new API as Im not sure if this is really needed and if it can't be solved on the client side with just a helper function this might be better but again I don't know the use-case so hard for me to say what would be the best solution.

@candiduslynx candiduslynx force-pushed the feat/custom-resolver-for-type branch from a332b37 to edcd8f2 Compare September 22, 2022 07:44
@candiduslynx
Copy link
Copy Markdown
Contributor Author

candiduslynx commented Sep 22, 2022

I'll rebase the code once #166 is merged, so there'll be only appropriate changes present in the PR.

@candiduslynx candiduslynx force-pushed the feat/custom-resolver-for-type branch from edcd8f2 to d848880 Compare September 22, 2022 07:59
@yevgenypats
Copy link
Copy Markdown
Contributor

yevgenypats commented Sep 22, 2022

Let's discuss live today but from quick glance seems to me we can keep our current convention and add WithPathResolverTransformer and have the same signature (for WithNameTransformer and WithTypeTransformer) that get's struct field and returns a string, error

@hermanschaaf
Copy link
Copy Markdown
Contributor

hermanschaaf commented Sep 22, 2022

I'm wondering whether we need to add WithPathResolverTransformer to the SDK, increasing its surface area. In theory, it should be possible to call codegen.NewTableFromStruct, then loop over the returned columns, updating the resolver to what you need it to be for special cases.

To be fair though, we probably could have done that for all the other transformers as well, so maybe this is fine since it will make plugin code more standardized.

@candiduslynx candiduslynx force-pushed the feat/custom-resolver-for-type branch from d848880 to 8b9fd6f Compare September 22, 2022 11:34
@candiduslynx candiduslynx changed the title feat(codegen): Add WithTypeResolver option feat(codegen): Add WithResolverTransformer option Sep 22, 2022
@github-actions github-actions bot added feat and removed feat labels Sep 22, 2022
@candiduslynx candiduslynx enabled auto-merge (squash) September 22, 2022 11:51
hermanschaaf
hermanschaaf approved these changes Sep 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@hermanschaaf hermanschaaf left a comment

Choose a reason for hiding this comment

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

LGTM 👍

yevgenypats
yevgenypats suggested changes Sep 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

Left some comments but looks good I think. cc @hermanschaaf @candiduslynx

@candiduslynx candiduslynx force-pushed the feat/custom-resolver-for-type branch from 0c52da7 to c36c474 Compare September 22, 2022 14:25
@candiduslynx candiduslynx requested review from yevgenypats and removed request for erezrokah September 22, 2022 14:27
@candiduslynx candiduslynx merged commit 9529956 into main Sep 22, 2022
@candiduslynx candiduslynx deleted the feat/custom-resolver-for-type branch September 22, 2022 20:21
@cq-bot cq-bot mentioned this pull request Sep 22, 2022
Merged
candiduslynx pushed a commit that referenced this pull request Sep 23, 2022
@cq-bot
chore(main): Release v0.8.1 (#170)
6d6ec00 
🤖 I have created a release *beep* *boop*
---


##
[0.8.1](v0.8.0...v0.8.1)
(2022-09-22)


### Features

* **codegen:** Add `WithResolverTransformer` option
([#164](#164))
([9529956](9529956))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@hermanschaaf hermanschaaf hermanschaaf approved these changes

@yevgenypats yevgenypats yevgenypats approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@candiduslynx candiduslynx

Labels

automerge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@candiduslynx @yevgenypats @hermanschaaf