Skip to content

Conversation

murdos
Copy link
Contributor

@murdos murdos commented Jun 10, 2021

Fixes #622

@sambsnyd sambsnyd requested review from jkschneider and sambsnyd June 11, 2021 02:34
@sambsnyd sambsnyd added the enhancement New feature or request label Jun 11, 2021
@sambsnyd sambsnyd added this to the 7.7.0 milestone Jun 11, 2021
@jkschneider jkschneider self-assigned this Jun 11, 2021
@murdos murdos marked this pull request as ready for review June 14, 2021 14:44

@Disabled
@Test
fun fromPackagePrivateToProtected_broken(jp: JavaParser) = assertChanged(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are tests / situations that are currently not correctly handled.
Some (all?) are already not supported by code in existing visitors (see links in the ticket + changes in HideUtilityClassConstructorVisitor).
I spent quite some time on it, but I haven't succeed in fixing them (this is my first incursion with AST ^^)

I think this PR is already an improvement, in the sense that it'll factorize the logic, and that all other visitors using it will benefit from improvements and future fix to this visitor.

@jkschneider jkschneider modified the milestones: 7.7.0, 7.8.0 Jun 15, 2021
@jkschneider
Copy link
Member

jkschneider commented Jun 15, 2021

Thank you so much for all the hard work on this @murdos! I made some further edits to support more use cases, so there is only one test still disabled now on main. And we can continue working from main. This PR is merged with a series of commits on main ending with 52f7c10.

Please check it over and see if I've correctly incorporated everything you added in the past couple days.

@github-actions
Copy link
Contributor

Unit Test Results

   431 files  ±0     431 suites  ±0   3m 44s ⏱️ ±0s
2 112 tests ±0  2 078 ✔️ ±0  34 💤 ±0  0 ❌ ±0 
3 005 runs  ±0  2 959 ✔️ ±0  46 💤 ±0  0 ❌ ±0 

Results for commit 52f7c10. ± Comparison against base commit 52f7c10.

@murdos
Copy link
Contributor Author

murdos commented Jun 15, 2021

Please check it over and see if I've correctly incorporated everything you added in the past couple days.

Thanks for polishing and having merged my work!
It seems that there are some divergences, mainly on tests, but nothing important. Your final version looks fine.

@murdos murdos deleted the java/change-method-access-level branch September 26, 2021 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New java recipe: ChangeMethodAccessLevel
3 participants