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

Add @PartFilename config - on/off, value, value suffix #452

Merged
merged 5 commits into from
Sep 5, 2023

Conversation

patr1kt0th
Copy link
Contributor

@patr1kt0th patr1kt0th commented Aug 28, 2023

Fixes: #435

@hbelmiro
Copy link
Contributor

@patr1kt0th for you to run the RESTEasy Reactive tests locally:

cd integration-tests
mvn clean install -Presteasy-reactive
mvn verify -Presteasy-reactive

@patr1kt0th
Copy link
Contributor Author

@hbelmiro thanks, I've already prepared a "fix" and split the test classes (classic & reactive), but I was/am fighting with a compilation error, so I'll try your suggestion this evening and let's see... will get back to you as soon as I have something new

@patr1kt0th
Copy link
Contributor Author

@hbelmiro, @ricardozanini, please check. It worked locally, so hopefully there won't be any problems.

But I have one question... I did the new config options in a similar way the others are done, but I was thinking whether it wouldn't be better to put all PartFilename related config under a part-filename key and use/rename them for example like this:

  • quarkus.openapi-generator.codegen.(spec.xyz).partfile-name.generate=true|false
  • quarkus.openapi-generator.codegen.(spec.xyz).partfile-name.value=<some-value>
  • quarkus.openapi-generator.codegen.(spec.xyz).partfile-name.add-field-name-to-value=true|false (because I don't like the part-filename-value-suffix that much)

What do you think?

Copy link
Contributor

@hbelmiro hbelmiro left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution @patr1kt0th. Very good job!
I just left a comment regarding where you placed the dependencies for the test.

And one picky suggestion: it seems you can avoid code duplication in tests. Maybe using parameterized tests or just extracting common parts to a different method. IDK, it's up to you and totally optional. If you want to leave it as it is, that's fine for me.

Comment on lines +20 to +27
<dependency>
<groupId>org.jboss.resteasy</groupId>
<artifactId>resteasy-multipart-provider</artifactId>
</dependency>
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-resteasy-reactive-common</artifactId>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

We declare dependencies that are specific for RESTEasy Reactive or Classic in its equivalent profile in integration-tests/pom.xml. I'm not sure about resteasy-multipart-provider, but you can move at least quarkus-resteasy-reactive-common to there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the info, I noticed it, but I did not want to put them there, as they're (only) relevant to the part-filename tests and I didn't want to influence the rest of the tests.

I need them to be able to compile the tests as they're checking both types of annotations and I couldn't figure out, how to do it in a more elegant way. Or maybe I'm just doing something wrong?

Btw. I think the quarkus-resteasy-reactive-common should already be part of quarkus-rest-client-reactive-jackson which you already have there.

@patr1kt0th
Copy link
Contributor Author

Thanks for the feedback, @hbelmiro. I fully agree with the picky suggestion... I didn't want to spend too much time with it, I assume. But I improved it a bit, please check.

I also fixed the typo and the README's format and I tried to implement the suggestion I made earlier = introducing the part-filename config group, but it did not work as I imagined it (it worked, but there were some warnings), so I just renamed the "suffix" config parameter.

@patr1kt0th
Copy link
Contributor Author

Oh, I just noticed I messed up the README.md file. @hbelmiro, can you please tell me what kind of MD settings you use in IntelliJ? Is it documented somewhere? Thanks.

@hbelmiro
Copy link
Contributor

hbelmiro commented Sep 3, 2023

@patr1kt0th we don't have any specific MD settings. As long as you don't mess it up too much, that's fine (which doesn't seem to be the case 😆).
I'll review it ASAP.

@patr1kt0th
Copy link
Contributor Author

Thanks @hbelmiro, I reverted the messed-up formatting and hopefully fixed all the issues it caused.

@patr1kt0th
Copy link
Contributor Author

Thanks for the review and the suggestions/fixes @ricardozanini 👍

@ricardozanini
Copy link
Member

@hbelmiro as your leave gift, can u take a final look?

@hbelmiro hbelmiro merged commit d9e9061 into quarkiverse:main Sep 5, 2023
12 checks passed
@hbelmiro
Copy link
Contributor

hbelmiro commented Sep 5, 2023

@all-contributors please add @patr1kt0th for code
@all-contributors please add @patr1kt0th for test

@allcontributors
Copy link
Contributor

@hbelmiro

I've put up a pull request to add @patr1kt0th! 🎉

github-actions bot pushed a commit that referenced this pull request Sep 5, 2023
* Add @PartFilename config - on/off, value, value suffix

* Split PartFilenameTest into classic and reactive

* Get rid of code duplicity in tests, fix README.md

* Revert messed up README.md formatting

* Apply suggestions from code review

Co-authored-by: Ricardo Zanini <[email protected]>

---------

Co-authored-by: Ricardo Zanini <[email protected]>
hbelmiro added a commit that referenced this pull request Sep 5, 2023
* Add @PartFilename config - on/off, value, value suffix (#452)

* Add @PartFilename config - on/off, value, value suffix

* Split PartFilenameTest into classic and reactive

* Get rid of code duplicity in tests, fix README.md

* Revert messed up README.md formatting

* Apply suggestions from code review

Co-authored-by: Ricardo Zanini <[email protected]>

---------

Co-authored-by: Ricardo Zanini <[email protected]>

* Replaced jakarta with javax
Fixed version

---------

Co-authored-by: Patrik Toth <[email protected]>
Co-authored-by: Ricardo Zanini <[email protected]>
Co-authored-by: Helber Belmiro <[email protected]>
@patr1kt0th
Copy link
Contributor Author

Thanks for your cooperation, guys 🤝

@patr1kt0th patr1kt0th deleted the issue-435 branch September 5, 2023 18:10
@hbelmiro
Copy link
Contributor

@all-contributors please add @patr1kt0th for code

@allcontributors
Copy link
Contributor

@hbelmiro

I've put up a pull request to add @patr1kt0th! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants