Provide indent-group feature - #120
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #120 +/- ##
==========================================
+ Coverage 96.19% 96.30% +0.10%
==========================================
Files 43 43
Lines 2945 3032 +87
==========================================
+ Hits 2833 2920 +87
Misses 96 96
Partials 16 16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // Returns an error unless the remainder is empty or closes a supported block comment. | ||
| func validateDeclarationRemainder(source string, prefix string) error { | ||
| remainder := strings.TrimSpace(source) | ||
| if remainder == "" || remainder == "*/" || remainder == "-->" { |
There was a problem hiding this comment.
Rejecting every trailing token is a breaking change, not just indent-group validation.
The old lookup matched names with a greedy "(.*)" and silently ignored whatever followed. Now anything that is not exactly */ or --> aborts fragmentation of the whole file. Markers that worked before this PR and fail after it:
- comment closers outside the two literals whitelisted here:
#}(Twig/Jinja),--}}(Handlebars),%>(JSP/ERB),*)(Pascal/OCaml),#>(PowerShell),]]>; - code or prose after the closer:
/* #docfragment "main" */ public void run() {,// #docfragment "main" (see the loop below).
None of the repo's own fixtures hit this, so CI stays green, but users hit it on upgrade with no indent-group in sight, and the failure mode is a hard error for the entire file rather than a skipped marker.
Two ways to keep the typo detection this buys without the collateral damage:
- only reject a remainder that is attribute-shaped (something like
^[A-Za-z][A-Za-z_-]*\s*=), and keep ignoring the rest as before; or - keep the strict rule, but hoist the accepted closers into a named list instead of two inline literals, and call the change out in the PR description and in
named-fragment.mdas deliberate.
| return nil, err | ||
| } | ||
|
|
||
| return declaration.names, nil |
There was a problem hiding this comment.
FindDocFragments drops declaration.indentGroup, and it is the only exported way to read an opening marker — both fragmentDeclaration and findDocFragmentDeclaration are unexported.
Two consequences:
- callers outside the package can parse a marker but cannot see the group it declares, even though the parser now understands it;
- the parsed group value is asserted nowhere in the unit tests.
should find fragment openings with an indentation group(fragmentation_test.go:627) only checks the names, so it would still pass ifparseIndentGroupreturned the wrong string; the only thing pinning the value is the end-to-end indentation test.
Consider exporting the declaration (e.g. FindDocFragmentDeclaration() (FragmentDeclaration, error)) and keeping FindDocFragments as a thin wrapper over it, so both the public API and the tests can observe the group.
| // error - when the name is not a valid quoted string. | ||
| func consumeQuotedName(source string) (string, string, error) { | ||
| quotedName, remainder := consumeQuotedValue(source) | ||
| name, err := unquoteName(quotedName) |
There was a problem hiding this comment.
unquoteName accepts "", so // #docfragment "" declares a fragment whose name is the empty string. That fragment can never be referenced: fragment="" on an <embed-code> instruction means "whole file" (embedding/parsing/instruction.go:58). The marker lines still vanish from the rendered content, so the typo is silent.
The behavior predates this PR, but the name parsing is being rewritten here, and parseIndentGroup rejects an empty value one function down. The same guard on the name would make the two consistent and turn a silent no-op into a diagnosable error.
| if len(line) < fragmentsStart { | ||
| return unquotedNames, fmt.Errorf( | ||
| "found `%s` prefix without any name", prefix, | ||
| declaration, err := parseFragmentDeclaration(line, prefix, false) |
There was a problem hiding this comment.
lookup is now a pass-through with a single caller (FindEndDocFragments), and prefix always receives FragmentEnd. unparam is enabled in .golangci.yml and reports exactly this shape on unexported functions (prefix always receives FragmentEnd), so this will likely fail the lint job.
The indirection no longer earns its keep either: FindEndDocFragments can call parseFragmentDeclaration(line, FragmentEnd, false) directly. That also retires a doc comment whose prefix example — for example "#docfragment", line 106 — is now unreachable, since start markers go through findDocFragmentDeclaration instead.
| @@ -105,17 +111,43 @@ func (f Fragment) text(lines []string, separator string) (string, error) { | |||
| // Returns: | |||
| // [][]string - selected lines grouped by partition. | |||
There was a problem hiding this comment.
Stale after the signature change: the function now returns []partitionText, not [][]string, and each element carries the indentation group as well as the lines.
| text := "" | ||
| for index, partitionText := range partitionsTexts { | ||
| cutIndentLines := indent.CutIndent(partitionText, indentation) | ||
| indentation := indentations[partitionText.indentGroup] |
There was a problem hiding this comment.
The loop variable partitionText on line 91 now shadows the partitionText type introduced at fragment.go:48, so the type name is unusable for the rest of the loop body and the reader meets partitionText.indentGroup (a value) a few lines after partitionText{...} (a type). Renaming the variable to partition, as commonIndentations already does, removes the collision.
|
|
||
| Add `indent-group="name"` to an opening `#docfragment` marker. The group name | ||
| must be a non-empty quoted string. The matching `#enddocfragment` marker does | ||
| not repeat the attribute. |
There was a problem hiding this comment.
"does not repeat the attribute" reads as optional, but the parser rejects it outright: #enddocfragment "x" indent-group="y" fails with indent-group is only supported by #docfragment (fragmentation/lookup.go:164).
Worth saying that repeating it is an error. Since this page is now the closest thing to a grammar for the marker, it is also the right place to state the other new rule — nothing may follow the fragment names except an optional indent-group and a */ or --> comment closer.
Oleg-Melnik
left a comment
Oleg-Melnik
left a comment
There was a problem hiding this comment.
@Vladyslav-Kuksiuk LGTM with comments to address.
2 participants
This PR provides
indent-groupfeature which allows to separate common indents inside onedocfragment.