Skip to content

Conversation

@Michaelll123
Copy link

What it does

Addressing issue #745
It is worth noting that the recommendation logic has been merged into the Eclipse (for Java). Here are the merged pull requests:

How to test

run testIfRecommend() in ExtractLocalVariableRefactoringTest.java.

Author checklist

@Michaelll123
Copy link
Author

@ruspl-afed @jld01 Please kindly review the pull request.

@jonahgraham
Copy link
Member

Thanks @Michaelll123 for providing the same behaviour change to CDT as you did for JDT.

Unfortunately no reviewers have found time to review it yet. Perhaps @jjohnstn may have some time to look at the code here too?

@jonahgraham jonahgraham added the editor The CDT C/C++, Assembly, Makefile and other editors provided by CDT label Oct 8, 2025
@jonahgraham jonahgraham force-pushed the NameRecommendationForExtractLocalVariable branch from 5bc252d to f44859d Compare October 14, 2025 15:35
@jonahgraham jonahgraham force-pushed the NameRecommendationForExtractLocalVariable branch from f44859d to 285bdb3 Compare October 14, 2025 15:46
@jonahgraham
Copy link
Member

This change looks reasonable, and as noted in OP a similar change was applied to JDT. However to accept this under the special review process for code I don't understand there needs to be tests for this code. The test provided in this PR does not fail if run against code base without the change here. This means that the test does not actually cover the changes made here. I don't know how to further review/proceed with this at the moment.

At a minimum, some test cases that demonstrate the new code contributed would be required.

Unfortunately this PR has been sat open for a while and none of the existing CDT committers that were requested for a review have prioritized this PR. Even if accepted I am concerned no one would be available to maintain this change (especially if there is a regression) going forward.

I am going to convert this to a draft and eventually close it if there is no further comments/contributions in this area.

@Michaelll123 I am sorry that this is the current conclusion of this PR. I am glad that you got the similar changes accepted in JDT and if you are interested in continuing on maintaining and contributing to CDT please let me know with a comment or an updated PR that contains tests that cover the new functionality.

@jonahgraham jonahgraham marked this pull request as draft October 14, 2025 16:07
@github-actions
Copy link

Test Results

  584 files  ± 0    584 suites  ±0   12m 34s ⏱️ - 1m 0s
9 888 tests + 1  9 864 ✅ + 1  24 💤 ±0  0 ❌ ±0 
9 889 runs   - 36  9 865 ✅  - 36  24 💤 ±0  0 ❌ ±0 

Results for commit 285bdb3. ± Comparison against base commit 7a5ac89.

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

Labels

editor The CDT C/C++, Assembly, Makefile and other editors provided by CDT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improving the Names Recommended by Extract Local Variable Refactoring.

2 participants