Remove dead fragmentation state and duplicate source reads#71
Conversation
…-unused-code # Conflicts: # indent/indent_test.go
| Configuration config.Configuration | ||
| SourcesRoot _type.NamedPath | ||
| CodeFile string | ||
| fragmentBuilders map[string]*FragmentBuilder |
| ) | ||
|
|
||
| const lastASCIIchar = 127 | ||
| var errUnsupportedEncoding = errors.New("unsupported source encoding: expected UTF-8") |
There was a problem hiding this comment.
Please capitalize the sentence and add a period at the end.
There was a problem hiding this comment.
This follows the Go error-handling convention: errors are combined into a single line using :.
| "return;", | ||
| })) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Consider removing this empty line.
There was a problem hiding this comment.
It is removed.
| ) | ||
|
|
||
| const lastASCIIchar = 127 | ||
| var errUnsupportedEncoding = errors.New("unsupported source encoding: expected UTF-8") |
There was a problem hiding this comment.
I've moved it into the method, and it is documented there.
There was a problem hiding this comment.
According to this comment: #71 (comment), it's better to use this error extracted(because it must be checked in another place), so I've added own type for it.
| type Fragmentation struct { | ||
| Configuration config.Configuration | ||
| SourcesRoot _type.NamedPath | ||
| CodeFile string |
There was a problem hiding this comment.
Why is this capitilized?
There was a problem hiding this comment.
It was part of the public API, but it no longer is, so we can change it to lowercase.
There was a problem hiding this comment.
Pull request overview
This PR simplifies the fragmentation/ path by removing unused fragmentation state, avoiding duplicate reads of source files during encoding validation, and relying on UTF-8 validation only (ASCII is a subset of UTF-8). It aligns with the intent of issue #58 by making source resolution and fragmentation easier to reason about.
Changes:
- Removed unused fragmentation state (
Fragmentation.Configuration,Fragmentation.SourcesRoot) and simplifiedNewFragmentationto take only a file path. - Reworked fragmentation to read a source file once (
os.ReadFile), validate UTF-8, then scan from memory. - Updated resolver/source selection to skip non-UTF-8 sources and added a regression test for trying the next code root.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| indent/indent_test.go | Removes an extraneous blank line from the indent test. |
| fragmentation/resolver.go | Replaces resolved-path struct with an absolute-path key; resolves sources by attempting cached fragmentation and skipping unsupported encodings. |
| fragmentation/fragmentation.go | Removes unused config/root fields and eliminates duplicate file reads by validating encoding on already-loaded content. |
| fragmentation/fragmentation_test.go | Adds a test ensuring non-UTF-8 sources are skipped in favor of the next configured code root; updates NewFragmentation call sites. |
| fragmentation/encoding.go | Simplifies encoding validation to UTF-8-only via utf8.Valid. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Oleg-Melnik
left a comment
Oleg-Melnik
left a comment
There was a problem hiding this comment.
@Vladyslav-Kuksiuk LGTM with Copilot’s comments addressed.
This PR removes unused fragmentation state and duplicate source reads, and simplifies encoding validation because ASCII is already valid UTF-8.
Resolves this issue.