Skip to content

feat(codegen): Allow consumers to override field types#139

Closed
erezrokah wants to merge 13 commits intocloudquery:mainfrom
erezrokah:feat/add_custom_types
Closed

feat(codegen): Allow consumers to override field types#139
erezrokah wants to merge 13 commits intocloudquery:mainfrom
erezrokah:feat/add_custom_types

Conversation

@erezrokah
Copy link
Copy Markdown
Member

@erezrokah erezrokah commented Sep 20, 2022

Summary

I guess this is a difference way to do #135 in order to fix write errors in the Azure plugin. See corresponding PR cloudquery/cloudquery#1885


Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

@erezrokah erezrokah marked this pull request as draft September 20, 2022 07:23
@erezrokah erezrokah marked this pull request as ready for review September 20, 2022 07:27
@erezrokah erezrokah changed the title feat: Allow consumers to override field types feat(codegen): Allow consumers to override field types Sep 20, 2022
@github-actions github-actions bot added feat and removed feat labels Sep 20, 2022
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.

This needs some more testing and I think we need to add the string comparison back, I recall there wasn't another way, maybe @hermanschaaf can weigh in as I dont remember on top of my head.

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.

I think this makes sense, and will enable us to handle a wide range of cases that we will eventually need to handle. It also seems better than trying to handle every possible type in the SDK.

I wonder if the reflect.Type passed in will be enough to handle all the cases though? What if the value type decision depended on the name of the field in a struct? Like maybe you'd want to convert things with Date in the name to timestamp, even though their Go type is string. For that reason, we may want to provide the full field reflect.StructField, rather than just the reflect.Type of the field.

Also, could you add a test case or two for this functionality? Nvm, I see you did a few minutes ago

@erezrokah
Copy link
Copy Markdown
Member Author

I wonder if the reflect.Type passed in will be enough to handle all the cases though? What if the value type decision depended on the name of the field in a struct? Like maybe you'd want to convert things with Date in the name to timestamp, even though their Go type is string. For that reason, we may want to provide the full field reflect.StructField, rather than just the reflect.Type of the field.

Good point I was wondering that too. Lets go with passing the field. It makes the code a bit let pretty but I think that's fine

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.

Looks good. Have one suggestion on the function type

kodiakhq bot pushed a commit to cloudquery/cloudquery that referenced this pull request Sep 21, 2022
@erezrokah erezrokah force-pushed the feat/add_custom_types branch from 486a85f to c8e4be3 Compare September 21, 2022 09:25
@erezrokah
Copy link
Copy Markdown
Member Author

I think I messed up the rebase, let me check this PR again

@erezrokah
Copy link
Copy Markdown
Member Author

@candiduslynx can you please check, I think I fixed the bad rebase I did and @yevgenypats please review

@erezrokah
Copy link
Copy Markdown
Member Author

Continued in #157

@erezrokah erezrokah closed this Sep 21, 2022
@erezrokah erezrokah deleted the feat/add_custom_types branch September 21, 2022 12:05
yevgenypats added a commit that referenced this pull request Sep 21, 2022
This is take two of #139

Trying to get right the `WithTypeTransformer` so we wont have to change
it in the near future as it's heavilly used external API.

I've also added error handling to all functions as we were basically
ignoring all of them and printing to the screen is not the right
approach catching those locally and in CI.

---

Use the following steps to ensure your PR is ready to be reviewed

- [ ] Read the [contribution guidelines](../blob/main/CONTRIBUTING.md)
🧑‍🎓
- [ ] Run `go fmt` to format your code 🖊
- [ ] Lint your changes via `golangci-lint run` 🚨 (install golangci-lint
[here](https://golangci-lint.run/usage/install/#local-installation))
- [ ] Update or add tests 🧪
- [ ] Ensure the status checks below are successful ✅

Co-authored-by: erezrokah <erezrokah@users.noreply.github.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
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.

5 participants