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

Feature/remove unused properties #4636

Merged
merged 6 commits into from
Nov 12, 2024
Merged

Conversation

nmck257
Copy link
Collaborator

@nmck257 nmck257 commented Nov 1, 2024

What's changed?

Added a new RemoveUnusedProperties maven recipe

What's your motivation?

Seems useful for certain hygiene tasks

Anything in particular you'd like reviewers to focus on?

My parentHasProperty method -- I think that it:

  • Can expect ~100% PomCache hits and not actually cause new downloads
  • Does not "poison" the pom cache / have weird side effects by resolving this fake, stubbed version of this project pom
  • Is the simplest impl I can think of

...but open to other ideas :)

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

Running this with the default .+ matcher can be a little over-eager, particularly for properties which support custom plugins, but the recipe seems quite practical for specific use cases (eg cleaning invalid .+\\.version properties for a Spring Boot project)

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

…PropertiesTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek
Copy link
Contributor

Great to see; I know this has come up as a request quite a bit over the past couple years, but it's always been tricky to get right. Have you given any though to properties used in resource filtering for instance? I feel that might complicate matters here.

@nmck257
Copy link
Collaborator Author

nmck257 commented Nov 1, 2024

zero thought into that! but at a glance, extending the scanner to check src/main/resources should be feasible? I can take a look next week

if (findPomUsagesVisitor.isAcceptable(sf, ctx)) { // ie: is a pom
findPomUsagesVisitor.visit(sf, ctx);
findFilteredResourcePathsVisitor.visit(sf, ctx);
} else if (!(tree instanceof JavaSourceFile)) { // optimization: avoid visiting code files which are almost always not filtered resources
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

my first instinct was to check the JavaSourceSet marker for this, but I didn't see a way to differentiate between code files and resources with that marker

@nmck257
Copy link
Collaborator Author

nmck257 commented Nov 7, 2024

fyi @timtebeek I'll probably merge this along later today, if no other concerns. I believe the added handling for filtered resources should be fairly robust

@nmck257 nmck257 merged commit a6c8d89 into main Nov 12, 2024
2 checks passed
@nmck257 nmck257 deleted the feature/RemoveUnusedProperties branch November 12, 2024 17:10
@timtebeek
Copy link
Contributor

Apologies for the delay in reviewing this @nmck257 ; I'd been out since last Wednesday and only just now trying to catch up. I did just spot perhaps a small oversight: the default delimiters for resource filtering include @a@ as well, which isn't picked up currently.

On my end to test this I have the Apache Maven repositories checked out with downloaded LSTs :

mod git clone moderne maven "Apache Maven"
mod build maven --download-only

Then once configured in the Moderne IntelliJ plugin:
image
It's fairly easy to preview the changes made to Apache Maven:
image

Ideally there's no false positives in such a run; whereas currently nearly all projects would see properties removed, which is perhaps a bit too enthusiastic (although not necessarily wrong).

Something to perhaps follow up, especially if you're similarly considering a run over there making changes with false positives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants