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

codegen: Remove dead comments #3647

Merged
merged 1 commit into from
Apr 9, 2024
Merged

Conversation

hughsimpson
Copy link
Contributor

@hughsimpson hughsimpson commented Mar 29, 2024

Starting to address #841

The goal here is to run some tests on the JSON serialisation and schema generation to ensure discriminator declarations or lack thereof are respected for various jsonLib+scala versions.

Writing the tests for this revealed a difference in behaviour between jsoniter-scala and circe behaviour that I was unaware of (namely that jsoniter-scala will happily accept a json with a missing array field, treating it as an empty array; whereas circe will fail (the latter of which is what I'd expect given an openapi spec with 'required: true' stipulated on the given array field)). I've decided to try to keep this pr smaller than originally intended, and introduce only the new tests; although I have made a follow-up to address that specific issue.

This pr does, however, fix the caching, which I apparently broke previously when permitting the output to range across multiple files (cachedCopyFile check needs to be given a different cacheFile for each input file). Honestly, I think this is sufficient justification on its own for inclusion of the new tests in CI

Edit: was merged in #3655

Now just removes some dead comments

@hughsimpson hughsimpson changed the title codegen: Tests [WIP] codegen: Tests (#841) Apr 4, 2024
@hughsimpson hughsimpson marked this pull request as ready for review April 4, 2024 15:45
@hughsimpson hughsimpson changed the title codegen: Tests (#841) codegen: Re-enable plugin tests + add json roundtrip tests (#841) Apr 5, 2024
@hughsimpson hughsimpson changed the title codegen: Re-enable plugin tests + add json roundtrip tests (#841) codegen: Enable plugin tests in CI & add json roundtrip tests (#841) Apr 5, 2024
@hughsimpson
Copy link
Contributor Author

Well looks like #3655 got merged ahead of this one 😅 but I'm leaving it open to remove those dead comments.

@hughsimpson hughsimpson changed the title codegen: Enable plugin tests in CI & add json roundtrip tests (#841) codegen: Remove dead comments Apr 9, 2024
@kciesielski
Copy link
Member

I was supposed to merge this PR, not #3655, my bad! 😞
Anyway, let's proceed, I'm sorry for the omission!

@kciesielski kciesielski merged commit 2359a46 into softwaremill:master Apr 9, 2024
23 checks passed
@hughsimpson hughsimpson deleted the tests branch April 10, 2024 17:37
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.

None yet

2 participants