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.

Sign up for GitHub

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

Jump to bottom

Add or remove imports #10

Merged
merged 3 commits into from
Aug 11, 2023
Merged

Add or remove imports #10

merged 3 commits into from
Aug 11, 2023

Conversation

timtebeek
Copy link
Contributor

No description provided.

@timtebeek timtebeek self-assigned this Aug 10, 2023
@timtebeek timtebeek mentioned this pull request Aug 10, 2023
Closed
timtebeek
timtebeek commented Aug 10, 2023
View reviewed changes
src/main/java/org/openrewrite/java/template/RefasterTemplateProcessor.java Outdated
Comment on lines 261 to 269
for (String import_ : imports) {
recipe.append(" maybeRemoveImport(\"" + import_ + "\");\n");
recipe.append(" maybeAddImport(\"" + import_ + "\");\n");
}
for (String import_ : staticImports) {
recipe.append(" maybeRemoveImport(\"" + import_ + "\");\n");
int dot = import_.lastIndexOf('.');
recipe.append(" maybeAddImport(\"" + import_.substring(0, dot) + "\", \"" + import_.substring(dot + 1) + "\");\n");
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was surprised we don't seem to separate imports, even though they end up OK into each of the before/after templates. That meant here I've had to both add and remove them maybe; which was good enough to get working, but not something that I'd be confident relying on.

@timtebeek
Copy link
Contributor Author

Do we see any reasonable ways to unit test this stuff here? Not looking to slow us down, but it'd be slightly nicer than having to publish to maven local.

@timtebeek
Copy link
Contributor Author

Also quickly tried out testing in #11

@knutwannheden
Copy link
Contributor

I would prefer if we could try to instead apply RemoveUnusedImports and ShortenFullyQualifiedTypeReferences. That would then also allow us to use fully qualified names everywhere in the template code and not require the template to be aware of any imports. That in turn has the benefit that there won't be any collisions when applying the template to some compilation unit, which may have a colliding import declared already. I could look into this, if it is urgent. But I think it should actually be rather simple (a matter of generating two doAfterVisit() calls), so if you want to give it a try...

@timtebeek
Copy link
Contributor Author

Didn't get to a proper fix here, but we do have the tests merged into this one such that we should be able to iterate more easily.
Or we can merge this as is, and work on a proper solution next week, to unblock @AlekSimpson .

@timtebeek timtebeek added the enhancement New feature or request label Aug 11, 2023
@timtebeek timtebeek marked this pull request as ready for review August 11, 2023 16:10
knutwannheden
knutwannheden approved these changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@knutwannheden knutwannheden left a comment

Choose a reason for hiding this comment

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

Let's merge it. I will look more thoroughly at it next week.

@knutwannheden knutwannheden merged commit 0a956cd into main Aug 11, 2023
@knutwannheden knutwannheden deleted the add_or_remove_imports branch August 11, 2023 18:53
@timtebeek
Copy link
Contributor Author

Let's merge it. I will look more thoroughly at it next week.

Appreciate it! I've added a test to quickly capture the current behavior just now in 0073350.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knutwannheden knutwannheden knutwannheden approved these changes

Assignees

@timtebeek timtebeek

Labels
enhancement New feature or request
Projects
OpenRewrite
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@timtebeek @knutwannheden