Autosort: skip commented collections; fix comment order under # quokka:sort#165
Open
neilberkman wants to merge 1 commit into
Open
Autosort: skip commented collections; fix comment order under # quokka:sort#165neilberkman wants to merge 1 commit into
neilberkman wants to merge 1 commit into
Conversation
…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.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related changes in autosort comment handling:
has_commentsskip for config-driven autosort..lineso# quokka:sortremains stable and idempotent on commented collections.Changes
Before the #155/#158 overhaul,
Autosort.run/2skipped config-driven autosort when a sortable collection contained comments: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?/2back, byte-equivalent to the pre-overhaul predicate atc73b2b7~1, and includes it in theAutosort.run/2skip condition.# quokka:sortis handled separately byCommentDirectivesviaAutosort.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/3by.line.sort_groups_sequentially/3prepends 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:sortinside the map. Sorting by line keeps leading comments with their keys and preserves idempotence.Tests
defstructatom-list form,defstructkeyword form, and schema blocks.# quokka:sortregression 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, andCHANGELOG.md.PR Checklist
/docs/*.mdfilemix formatand committed any changes