kbuntrock / openapi-maven-plugin

Generate openapi documentation for SpringMVC or JaxRS/JakartaRS projects.
https://kbuntrock.github.io/openapi-maven-plugin/
MIT License
13 stars 8 forks source link

#100 Sort tags #101

Closed magx2 closed 4 months ago

magx2 commented 6 months ago

I've added sorting tags to get more predictable results for generating JSON/YML.

see https://github.com/kbuntrock/openapi-maven-plugin/issues/100

kbuntrock commented 6 months ago

Hello, thank you for your contribution, I appreciate it (first external one 🥳 )

I have a bunch of remarks about it tho' :

And finally, I'm trying to test anything I'm adding to this project in order to limit as much as possible any possible regression when improving this plugin. That's why I would like a unit test about this ordering process.

Thank you by advance!

Kind regards, Kevin

magx2 commented 6 months ago

6 unit tests fails.

ah, you do not have github actions configured?

magx2 commented 6 months ago

Tag comparison : to remove any possibility of comparison difference between linux and windows, I suggest to ignore whitespaces during comparison of computed names.

why white spaces? Did you mean case?

magx2 commented 6 months ago

sorting should not be done in getters

I've change List to SortedSet

magx2 commented 6 months ago

Why it's failing on ci/cd, but when I'm running locally mvn clean install it does not fail?

kbuntrock commented 6 months ago

ah, you do not have github actions configured?

It was not configured for PR coming from forks. But I changed the configuration and it seems to work with your PR now.

why white spaces? Did you mean case?

No, I meant white spaces. Sometimes linux and windows string comparison defer if they rely on the OS default "collation" (spaces count by default on one and not on the other). But the java implementation seems consistent, let's put away this suggestion and not complicate the code.

Why it's failing on ci/cd, but when I'm running locally mvn clean install it does not fail?

Weird, it fails also for me in local. Are you sure you don't deactivate tests with a maven option?

magx2 commented 6 months ago

Weird, it fails also for me in local. Are you sure you don't deactivate tests with a maven option?

Have you done git pull?

magx2 commented 6 months ago

nvm, I had turned test off in IntelliJ (when I was debugging previous bug I've turned it off)

magx2 commented 6 months ago

Ready 🚀!

magx2 commented 5 months ago

Do we have anything pending here?

magx2 commented 5 months ago

LGTM!

magx2 commented 4 months ago

Ready to merge?

kbuntrock commented 4 months ago

A few things annoys me with this one

Ready to merge?

Back on it. I remind having an issue with it but it seems I have not write it down. Re-reading this PR.

kbuntrock commented 4 months ago

Got the time yesterday to retrieve what was bothering me : In case of a name collision of the simple class name, using a Set will erase randomly some tags. A similar issue can be found with Enpoints having path collisions.

Even if we will probably generate an invalid documentation, we cannot hide the problem under the carpet (french expression :P)

Since I was on it, I took the liberty to derive from your branch to add my modifications : #109 I also "manually" set the order for operation types as described in #100

I let you review my modifications and if it please you, I'll merge this on Tuesday evening.

magx2 commented 4 months ago

OK, added few comments