Skip to content

Conversation

@arnavsharma990
Copy link
Contributor

This PR adds new IO test cases for OrganizeImports to capture the incorrect handling of inline comments (e.g., comments to the right of imports being lost, reordered incorrectly, or not associated with the correct import). These test
s encode the expected behavior so the underlying rule can later be updated to preserve inline comments consistently.

Locally, the new tests pass, but some unrelated expect*/testOnly suites occasionally behave inconsistently on my machine (sometimes showing passed, canceled, or failed). I’ve attached a screenshot in the discussion, but this inconsistency does not involve the new OrganizeImports tests themselves.

Please review the added test cases and confirm whether this structure matches what the maintainers expect before I proceed to implement the actual fix to OrganizeImports.
Screenshot 2025-11-28 at 4 17 07 PM
Screenshot 2025-11-28 at 4 04 01 PM

@arnavsharma990
Copy link
Contributor Author

Test cases included

I added failing test cases in the following files:

InlineCommentMultiple.scala

InlineCommentMoves.scala

InlineCommentRemoved.scala

These tests capture scenarios where inline comments are not preserved, shifted incorrectly, or dropped during import organization.

@arnavsharma990
Copy link
Contributor Author

arnavsharma990 commented Nov 28, 2025

@bjaglin The failing CI is expected as the new IO tests intentionally fail because they capture the inline comment issue described in #2267.
These failing tests demonstrate the bug before implementing the fix.

@tgodzik
Copy link
Contributor

tgodzik commented Dec 2, 2025

I think the test cases look fine, we might not need that many though.

@arnavsharma990
Copy link
Contributor Author

Thanks for the clarification and for reviewing the tests!
I’ll trim the test cases as suggested.

Also, should I start working on the actual fix for the issue now?

@tgodzik
Copy link
Contributor

tgodzik commented Dec 2, 2025

Sure, just make sure that if you use AI that you properly vet the output and try to minimize the changes.

Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

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

That's a great start 👍 I think we could add heading/trailing examples, as well as multi-line /* */ comments to have a better coverage.

Feel free to start working on implementation.

import c.C

object InlineCommentMultiple {
val keep = 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we can remove that if removeUnused is not tested

…aloneComment.scala

Co-authored-by: Brice Jaglin <bjaglin@gmail.com>
Copilot AI review requested due to automatic review settings December 3, 2025 15:51
Removed unused import 'c.C' to clean up code.
Copilot finished reviewing on behalf of arnavsharma990 December 3, 2025 15:54
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds new IO test cases to capture the current (incorrect) behavior of the OrganizeImports rule when handling inline comments. The tests document scenarios where inline comments are lost, reordered incorrectly, or not properly associated with their imports, establishing a baseline for future fixes to issue #2267.

  • Adds fixture file InlineCommentFixtures.scala with test packages and classes
  • Creates 4 test scenarios covering standalone comments, inline comment preservation during sorting, inline comment behavior with unused import removal, and inline comment movement
  • Establishes expected behavior patterns for subsequent OrganizeImports rule improvements

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
scalafix-tests/shared/src/main/scala/test/organizeImports/InlineCommentFixtures.scala Provides fixture classes in packages a, b, c, z, my.pkg, other, x, y for testing import organization with inline comments
scalafix-tests/input/src/main/scala/test/organizeImports/StandaloneComment.scala Tests that standalone comments (not inline) are preserved when imports are sorted
scalafix-tests/output/src/main/scala/test/organizeImports/StandaloneComment.scala Expected output showing standalone comment preserved with sorted imports
scalafix-tests/input/src/main/scala/test/organizeImports/InlineCommentRemoved.scala Tests inline comment behavior when associated import is removed as unused
scalafix-tests/output/src/main/scala/test/organizeImports/InlineCommentRemoved.scala Expected output showing unused import and its inline comment are both removed
scalafix-tests/input/src/main/scala/test/organizeImports/InlineCommentMultiple.scala Tests preservation of multiple inline comments during import sorting
scalafix-tests/output/src/main/scala/test/organizeImports/InlineCommentMultiple.scala Expected output showing inline comments move with their associated imports when sorted
scalafix-tests/input/src/main/scala/test/organizeImports/InlineCommentMoves.scala Tests inline comment staying with its import when imports are reordered
scalafix-tests/output/src/main/scala/test/organizeImports/InlineCommentMoves.scala Expected output showing inline comment moved with its import during sorting

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

import a.A // anote

object InlineCommentMultiple {
val keep = 1
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The imports a.A, b.B, and c.C are declared but not used in the object body (only val keep = 1 is defined). Without explicitly setting removeUnused = false in the configuration, these imports may be removed during the test execution if unused import removal is enabled by default. Consider either:

  1. Adding OrganizeImports.removeUnused = false to the configuration block, or
  2. Using the imported classes in the object body (e.g., val keep: A = ???)
Suggested change
val keep = 1
val keep = 1
val keepA: a.A = ??? // use a.A to keep import
val keepB: b.B = ??? // use b.B to keep import

Copilot uses AI. Check for mistakes.
import a.A

object InlineCommentMoves {
val keep = (null: AnyRef)
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The imports z.Z and a.A are declared but not used in the object body (only val keep = (null: AnyRef) is defined). Without explicitly setting removeUnused = false in the configuration, these imports may be removed during the test execution if unused import removal is enabled by default. Consider either:

  1. Adding OrganizeImports.removeUnused = false to the configuration block, or
  2. Using the imported classes in the object body (e.g., val keep: A = ???)
Suggested change
val keep = (null: AnyRef)
val keep: A = ??? // use imported class A to prevent unused import removal

Copilot uses AI. Check for mistakes.
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