Skip to content
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

Fix: Resolve date formats handling #231

Merged
merged 4 commits into from
Jun 18, 2024
Merged

Fix: Resolve date formats handling #231

merged 4 commits into from
Jun 18, 2024

Conversation

TheSadlig
Copy link
Contributor

Hello hello,

In was in need for a fix for #222, so here is a proposition.

I chose to use https://pkg.go.dev/cloud.google.com/go/civil to simplify everything, but another way could have been to implement a struct with a specific marshaller.

Let me know if this works for you,

Thanks,
TheSadlig

As there is already a processing from 'imports' official golang tools,
the uneeded imports are automatically wiped out at generation. We can
just add necessary imports in the 'imports.tmpl' files and the ones
that are not needed will be automatically removed.
@lerenn
Copy link
Owner

lerenn commented Jun 9, 2024

Really nice work @TheSadlig ! ☺️

I'm all for the use of an external library (less is more in code) which is widely used (so pretty well tested).

I just added to modification :

  • a3ba379: I remove the requiredImports logic from the project, because it is redundant as the imports library from the official tools is used in pkg/codegen/codegen.go removes any unnecessary imports from generated code. So I just added the "cloud.google.com/go/civil" in the imports.tmpl files and it will be automatically be removed if unused in the go file.
  • 67d18bc: As I removed the logic for requiredImports, no code where calling the function FieldValueExists. I could have removed it as well, but I think it will be really useful one day, so I wanted to keep it and unsure that any modification would not change its behavior and added tests.
  • cf40709: I fixed the test for AsyncAPI v3 because it was generated as an AsyncAPI v2.

If that's good for you, I'll merge everything but let me know if one of the modification I've made is problematic for you :)

@TheSadlig
Copy link
Contributor Author

@lerenn Sorry for the late reply.

That's perfect, thanks a lot, I especially like the fact that we don't have to check manually whether the import will be required !

TheSadlig

@lerenn lerenn merged commit 59071bb into lerenn:main Jun 18, 2024
4 checks passed
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.

3 participants