Refactor: Extract importer file index module#381
Conversation
Pull pipeline performance —
|
| Stage | Wall time | Resume attempts | Status | Details |
|---|---|---|---|---|
playground-sqlite-db-pull |
9.60 s | 1 | ✓ | condition=db-pull in PHP.wasm runtime=php.wasm 8.3 wp_mysql_parser=enabled mode=lexer native_lexer=verified native_token_stream=WP_MySQL_Native_Token_Stream native_token_count=18 native_parser=selected |
playground-sqlite-db-apply |
3.67 s | 1 | ✓ | condition=db-apply to SQLite in PHP.wasm runtime=php.wasm 8.3 wp_mysql_parser=enabled mode=parser native_lexer=verified native_token_stream=WP_MySQL_Native_Token_Stream native_token_count=18 native_parser=verified native_ast=WP_MySQL_Native_Parser_Node sqlite_driver_parser=verified |
| Total | 13.27 s |
Numbers carry runner noise; treat single-run deltas as directional, not authoritative.
📈 Trunk performance history — commit-by-commit timeline.
9595f63 to
987a75b
Compare
1124a7a to
9ea5c0f
Compare
987a75b to
27c2b08
Compare
9ea5c0f to
6a77548
Compare
d2b5891 to
c15b139
Compare
6a77548 to
d220669
Compare
c15b139 to
493054b
Compare
d220669 to
2b4f358
Compare
| use RuntimeException; | ||
| use function WordPress\Reprint\Exporter\assert_valid_path; | ||
|
|
||
| final class IndexLineParser |
There was a problem hiding this comment.
This class seems to exist solely as a namespace for the two methods. It doesn't use any internal state and it cannot be extended. Should we just go with two standalone functions? I'm worried about creating a proliferation of small classes.
| if ($this->index_store === null) { | ||
| $this->index_store = new IndexStore( | ||
| $this->index_file, | ||
| $this->index_updates_file, | ||
| [$this, "audit_log"], | ||
| ); | ||
| } |
There was a problem hiding this comment.
Before this PR, I could read this 12 lines long method and understand everything about it. With this PR, I need to read multiple methods, another class, and mentally keep track of additional mutable state. I'm not convinced this is helpful
| } | ||
| } | ||
| return null; | ||
| return $this->index_store()->readIndexLine($handle); |
There was a problem hiding this comment.
Pre-existing methods in this repo already use snake_case. Methods introduced in the new classes use camelCase. Let's stick to snake_case since that's what WordPress does.
| return $this->index_store; | ||
| } | ||
|
|
||
| private function remote_index_prefix_matcher(): IndexPathPrefixMatcher |
There was a problem hiding this comment.
When we're introducing new code, let's make sure it comes with informative doc blocks that explain in simple terms to a new reader what the code does. It makes navigating the codebase much easier without analyzing all the calls. Unfortunately, I haven't found a good way to get LLMs to consistently do that—they often write trivialities or rewrite the code in plain english. The best prompt I found so far is "document code like Linus Torvalds," but I still often spend time rewriting and expanding these comments to explain the intent, provide code examples etc.
One of the best documented classes I know is WP_HTML_Processor, largely thanks to @dmsnell. I always keep that in mind as an ideal model.
| "size" => (int) ($data["size"] ?? 0), | ||
| "type" => (string) ($data["type"] ?? "file"), | ||
| ]; | ||
| return IndexLineParser::parse($line); |
There was a problem hiding this comment.
We could also remove the entire private function and just call a global parse_index_line( $line ) directly. That being said, it's a pretty small function. Are we gaining much by removing it one step from the class that also determines the shape of that index? Looking at this a few lines below, I don't think OOP is serving us well here:
$this->index_store()->beginUpdates();
$this->index_updates_file = $this->index_store()->updatesFile();
Perhaps we could extract the entire indexing stage of the pipeline as the sole new class with just one responsibility?
| : 256 * 1024 * 1024; | ||
|
|
||
| $sorter = new \ExternalMergeSort( | ||
| new IndexLinePathKeyExtractor(), |
There was a problem hiding this comment.
Ditto as other comments – we're swapping a tiny inline function for another class living in another file that delegates work into yet another class, I'd say this is adding cognitive effort, not decreasing it.
2 participants
TBD