search

iheartradio / play-swagger

Swagger spec generator for play framework
Apache License 2.0
403 stars 131 forks source link

Remove default tags from global level #525

Closed kchaitanya1195 closed 1 year ago

kchaitanya1195 commented 1 year ago

Removing code for generating global tags object (https://github.com/iheartradio/play-swagger/issues/70 ). Also added an extra test to make sure that a "routes" tag is not added when a default routes file is used.

cc: @kailuowang

kchaitanya1195 commented 1 year ago

I had to add a couple of dependencies in order to run tests. I'm not sure how the existing CI is passing. I could use some pointers on how to investigate this.

cc: @kailuowang @Javakky-pxv

Javakky-pxv commented 1 year ago

https://github.com/JodaOrg/joda-time

Javakky-pxv commented 1 year ago

https://github.com/google/error-prone

Javakky-pxv commented 1 year ago

@kailuowang Can I ask you to review this PR as I do not have sufficient knowledge of this PR?

kailuowang commented 1 year ago

@kailuowang Can I ask you to review this PR as I do not have sufficient knowledge of this PR?

Sorry, I pretty much forgot about the code as well. I think the expected behavior is that the code generates a tag for each route from the path name of the route file in which the route is defined. That means that routes defined in the root route file will have a pretty useless tag of the root file path. It will be ideal to remove this tag for the root file but retain the tags for routes in referenced routes files.

kchaitanya1195 commented 1 year ago

@kailuowang I raised another pr a few years back and lost track of it. It got auto-closed due to no activity. You can check it for reference. https://github.com/iheartradio/play-swagger/pull/342

Javakky-pxv commented 1 year ago

70

Javakky-pxv commented 1 year ago

@kailuowang I am in favor of this change. It is a BREAKING CHANGE, but I am not sure if this is a milestone for ver 1.0.0, and I am considering releasing it as 0.14.0 and advertising it in the CHANGE LOG.

By the way, do you have a roadmap for ver 1.0.0? Or is it already substantially 1.0.0?

kailuowang commented 1 year ago

Imo, it should've been 1.0 by now. I don't have a roadmap. I stopped using swagger or play long time ago, and thus not a stakeholder of this lib. I don't think I have anything to say in regards to it's features or roadmap. One thing you might want to consider is a roadmap to scala 3. A big portion of reflection based code might needs to be rewritten.

On Tue, Mar 7, 2023, 12:19 AM Kazuma Iemura @.***> wrote:

@kailuowang https://github.com/kailuowang I am in favor of this change. It is a BREAKING CHANGE, but I am not sure if this is a milestone for ver 1.0.0, and I am considering releasing it as 0.14.0 and advertising it in the CHANGE LOG.

By the way, do you have a roadmap for ver 1.0.0? Or is it already substantially 1.0.0?

— Reply to this email directly, view it on GitHub https://github.com/iheartradio/play-swagger/pull/525#issuecomment-1457554628, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUKOMH3P3SWG7UJGP45X3W23AO7ANCNFSM6AAAAAATNSKTJY . You are receiving this because you were mentioned.Message ID: @.***>

Javakky-pxv commented 1 year ago

@kchaitanya1195 @kailuowang Sorry for the delay. I will use this change as an opportunity to change the major version to 1.0.0. I will update the README soon, but it will likely stay the same until I get around to it ......

Support for Scala 3 is an important change, but I will take the plunge and move up to 2.0.0 if there are major changes at that time.