Skip to content

Autosort: skip commented collections; fix comment order under # quokka:sort#165

Open
neilberkman wants to merge 1 commit into
emkguts:mainfrom
neilberkman:fix/restore-has-comments-skip
Open

Autosort: skip commented collections; fix comment order under # quokka:sort#165
neilberkman wants to merge 1 commit into
emkguts:mainfrom
neilberkman:fix/restore-has-comments-skip

Conversation

@neilberkman

Copy link
Copy Markdown
Collaborator

Summary

Two related changes in autosort comment handling:

  1. Restore the pre-2.13.0 has_comments skip for config-driven autosort.
  2. Keep returned comments sorted by .line so # quokka:sort remains stable and idempotent on commented collections.

Changes

Before the #155/#158 overhaul, Autosort.run/2 skipped config-driven autosort when a sortable collection contained comments:

should_skip || has_comments || !is_sortable -> {:cont, ...}

That meant autosort: [:map, :defstruct, :schema] did not reorder commented maps, defstructs, or schema blocks. The overhaul removed that guard and instead sorted commented collections while trying to keep comments attached to their keys.

That works for per-key comments, but not for standalone section comments heading a run of entries. Sorting alphabetically disperses the run, and the section header ends up next to whichever key now follows it. The section-header and per-key-comment cases are structurally identical at the AST/comment-layout level, so there is no reliable heuristic that separates them.

This PR adds has_comments_inside?/2 back, byte-equivalent to the pre-overhaul predicate at c73b2b7~1, and includes it in the Autosort.run/2 skip condition. # quokka:sort is handled separately by CommentDirectives via Autosort.sort/2, so users can still opt in per value when they want a commented collection sorted.

This PR also sorts the comments returned by sort_keyword_list_with_comments/3 by .line. sort_groups_sequentially/3 prepends placed comments while iterating, and the no-reorder branch could also return comments out of order. Since the formatter emits comments in list order, that could reverse and cluster per-key comments, and on a second pass pull # quokka:sort inside the map. Sorting by line keeps leading comments with their keys and preserves idempotence.

Tests

  • Updated the three existing commented-map autosort tests to assert the restored skip behavior.
  • Added skip-regression tests for section comments, defstruct atom-list form, defstruct keyword form, and schema blocks.
  • Added a # quokka:sort regression test for multiple per-key leading comments, directive preservation, and idempotence.

Full suite: 484 tests, 0 failures.

Docs

Updated docs/autosort.md, docs/comment_directives.md, and CHANGELOG.md.

PR Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • All new and existing tests pass
  • If there are changes to installation, usage, or project overview, I have updated README.md
  • If there are changes to styling, I have updated the relevant /docs/*.md file
  • I have run mix format and committed any changes

…a:sort

Two related changes in the autosort comment-handling path:

1. Restore the pre-2.13.0 `has_comments` skip for config-driven autosort.

   Before the overhaul (emkguts#155/emkguts#158), Autosort.run/2 had this guard:

     should_skip || has_comments || !is_sortable -> {:cont, ...}

   i.e. `autosort: [:map | :defstruct | :schema]` did not reorder a
   collection that contained comments. emkguts#155 removed it and emkguts#158 replaced
   it with sorting the collection and trying to move comments with their
   keys. That works for per-key comments. For a standalone comment that
   heads a run of entries (the common real-world pattern in config,
   i18n, and large fixtures), sorting alphabetically disperses the run
   and the header ends up next to whatever key now follows it.

   The two patterns are structurally identical at the AST + comment
   level, so a heuristic can't distinguish "comment for one key with an
   uncommented neighbour" from "section header over many keys" without
   breaking the per-key case the overhaul intended to support.

   Adds `has_comments_inside?/2` back and includes it in the run/2 skip
   cond. `# quokka:sort` is handled separately (CommentDirectives calls
   Autosort.sort/2 directly) and is unaffected, so users who really want
   a commented collection sorted opt in per-value.

2. Sort the comments list returned by `sort_keyword_list_with_comments`
   by `.line`.

   `sort_groups_sequentially` prepends placed comments per iteration,
   so the returned list was in reverse line order. The else branch (no
   reorder needed) likewise returned `pair_comments ++ rest` unsorted.
   The formatter emits comments in list order, so on a `# quokka:sort`
   over a map with multiple per-key leading comments, the per-pair
   comments rendered reversed and clumped above one key, and on a
   second pass the directive comment was pulled inside the map. Sorting
   the returned list by line fixes both: leading comments stay with
   their keys after sort, and `# quokka:sort` stays above the map and
   is idempotent.

Test changes:
- Three existing tests that asserted config autosort reorders commented
  maps now assert the restored behaviour: the map is left untouched and
  mix format's end-of-line comment hoist still applies.
- New skip-regression tests for: map with a section comment over
  multiple keys, defstruct atom-list with a comment, defstruct keyword
  form with a comment, schema block with a comment.
- New `# quokka:sort` regression test asserts multiple per-key leading
  comments travel with their keys and the directive is preserved.

Docs/changelog updated to describe the restored rule and point users at
`# quokka:sort` as the per-value opt-in for commented collections.
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.

1 participant