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

Remove default tags from global level #525

Merged
merged 3 commits into from
Mar 10, 2023

Conversation

kchaitanya1195
Copy link
Contributor

Removing code for generating global tags object (#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
Copy link
Contributor Author

kchaitanya1195 commented Jan 3, 2023

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
Copy link
Collaborator

@Javakky-pxv
Copy link
Collaborator

@Javakky-pxv
Copy link
Collaborator

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


val tagsJson = mergeByName(generatedTagsJson, (baseJson \ "tags").asOpt[JsArray].getOrElse(JsArray()))

val tagsJson = (baseJson \ "tags").asOpt[JsArray].getOrElse(JsArray())
Copy link
Collaborator

Choose a reason for hiding this comment

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

It has been a long time since I looked at this code base. It is unclear to me how this change can retain the tags generated from file paths? The test was also changed. The code is generating new set of tags now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want to retain tags generated from file paths because we don't need them. Please check our chat on this thread.

The code is not generating new tags. The previous was only testing if a small subset of tags was present in the output file. I made that set exhaustive.

@kailuowang
Copy link
Collaborator

@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
Copy link
Contributor Author

@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.
#342

Copy link
Collaborator

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

It looks fine to me. But I'll leave the final decision to @Javakky-pxv
It's still a breaking change, we might want to make it loud in the release note and bump up the minor version number

@Javakky-pxv
Copy link
Collaborator

#70

@Javakky-pxv
Copy link
Collaborator

@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
Copy link
Collaborator

kailuowang commented Mar 7, 2023 via email

@Javakky-pxv
Copy link
Collaborator

@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.

@Javakky-pxv Javakky-pxv merged commit de25a72 into iheartradio:master Mar 10, 2023
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