Skip to content

[V2:02] Split Preprocessor CLI and Command Modules - #16

Merged
SamWilsn merged 2 commits into
eips-wg:masterfrom
ritovision:v2/02-module-cli-groundwork
May 9, 2026
Merged

[V2:02] Split Preprocessor CLI and Command Modules#16
SamWilsn merged 2 commits into
eips-wg:masterfrom
ritovision:v2/02-module-cli-groundwork

Conversation

@ritorhymes

@ritorhymes ritorhymes commented May 5, 2026

Copy link
Copy Markdown
Contributor

Part of V2: Multi-repo local build system.

Summary

This PR establishes the preprocessor module boundaries that the rest of the V2 stack builds on. It is a behavior-preserving split of main.rs into owned modules (cli, context, layout, changed), so review should focus on where code lives rather than what it does.

  • Move the clap command surface and command argument types into cli.rs.
  • Move root path resolution into context.rs.
  • Move shared build layout constants into layout.rs.
  • Move changed-file command execution and the shared is_proposal_path classifier into changed.rs.
  • Add initial module/test ownership notes in src/README.md.

Review Notes

This is the module foundation for the preprocessor stack. Later preprocessor PRs build on these extracted modules rather than continuing to grow main.rs.

Review this as a structural split of existing behavior. CLI surface and runtime behavior should remain unchanged.

Prepared::prepare now calls changed::is_proposal_path; the helper moved out of Prepared because both the changed command and the build pipeline filter on proposal paths.

Feature-specific command behavior still lands with the later PRs that expose those features.

Verification

  • Review main.rs for reduced top-level orchestration.
  • Review cli.rs for the moved clap command surface.
  • Review changed.rs, context.rs, and layout.rs for extracted helper ownership.
  • Review src/README.md for the initial module and test ownership guidance.

@ritorhymes ritorhymes changed the title Split Preprocessor CLI and Command Modules [V2:02] Split Preprocessor CLI and Command Modules May 5, 2026
Comment thread src/README.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is somewhat for/by LLMs? I've seen people use .claude or .agents. Would that be more appropriate?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's intended to be useful for humans and agents alike.

I do think agent-first guidance files would be useful to include: an AGENTS.md in each repo and for the generated workspace to serve as entrypoints routing to other md files as needed, perhaps inside .agents/.

If there were dedicated folders like .agents/, I'm not sure I would put that particular file directly in one unless we committed to consolidating broader general documentation that humans are meant to consume there too.

I would like to think more about this, but I do not want it to block this review process. I think it would be better to confirm how the architecture stands first, then build an agent guidance system around the final shape so it can help with maintenance and contributions without getting anchored to transitional scaffolding.

Comment thread src/README.md Outdated
- `context.rs` owns command input path resolution.
- `layout.rs` owns shared build layout names.

Other modules keep their existing domain ownership.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is only relevant while the pull request is in flight. After merge, it loses meaning. Existing when?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in the latest push.

Comment thread src/changed.rs Outdated

pub(crate) fn is_proposal_path(p: PathBuf) -> bool {
// Only lint `content/00001.md` and `content/00001/index.md` files.
let mut p = p.to_path_buf();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unnecessary clone.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good catch. I'll drop the clone by making the argument mutable directly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in the latest push.

Comment thread src/layout.rs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file could be a mod layout { ... } instead. Doesn't seem to get much more complex in the later PRs either.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In V2:02 here, this is a small constants module because that PR is still laying scaffolding for later runtime boundaries. I kept it as a file module because it becomes shared prepared-runtime layout vocabulary once V2:10 lands.

By V2:10, layout.rs owns helpers like output_path, mounted_theme_path, and theme_config_path. By V2:18, layout::* is imported across unrelated modules like find_root, git, pipeline, proposal, serve, and zola, while still having no dependencies of its own. So I do not think there is a natural file to nest it inside as mod layout { ... } without assigning it an arbitrary parent.

Comment thread src/context.rs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an argument to be made for putting the *_DIR constants, root and is_proposal_path into one module dedicated to paths. What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is one of the awkward parts of slicing the system up for review. In V2:02, layout.rs is mostly scaffolding for boundaries that become clearer later, so it looks heavier than the immediate code requires.

I think the later stack justifies keeping the split. By V2:10, layout.rs owns the shared prepared-runtime layout helpers like output_path, mounted_theme_path, and theme_config_path. Separately, context.rs grows into CLI/workspace root and input-path resolution.

The proposal-path part follows the same pattern. In V2:02, changed.rs carrying is_proposal_path is narrower than the eventual ownership boundary. The later stack moves that logic toward the proposal domain rather than a generic paths module: proposal.rs starts owning proposal-number and proposal-path classification in V2:13, and by V2:18 changed.rs calls proposal::is_proposal_path instead of carrying that logic itself.

I do want to fix things to ensure a stable and scalable final architecture, but I am trying not to over-optimize intermediate scaffolding when the later stack gives the code a clearer home. I can try to adjust scaffolding where it materially improves reviewability within reason, but there is no perfect way to split a system like this without tradeoffs, and chasing that across every intermediate branch is not really feasible.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The later stack moves that logic toward the proposal domain rather than a generic paths module: proposal.rs starts owning proposal-number and proposal-path classification in V2:13, and by V2:18 changed.rs calls proposal::is_proposal_path instead of carrying that logic itself.

Ah, that makes sense. I was questioning whether is_proposal_path in changed.rs made sense.

Move the existing clap command surface into cli.rs, root resolution into
context.rs, and build layout constants into layout.rs.

Move changed-file execution and the shared is_proposal_path classifier
into changed.rs while preserving existing runtime behavior.

Keep main.rs focused on top-level orchestration and add initial
module/test ownership notes for later preprocessor slices.
@ritorhymes

Copy link
Copy Markdown
Contributor Author

@SamWilsn I am open to feedback on how to make the stack easier to review as a stack, especially where an intermediate slice looks awkward before later PRs give it its final owner. I do not want to churn every transitional boundary just to make each slice look perfect in isolation, but if there are recurring patterns that make review harder and you have actionable feedback I can apply, I would like to know so I can address those deliberately.

@ritorhymes
ritorhymes force-pushed the v2/02-module-cli-groundwork branch from bf79ec7 to a94c9fb Compare May 9, 2026 02:21
@SamWilsn

SamWilsn commented May 9, 2026

Copy link
Copy Markdown
Contributor

I'll try to pay more attention to the future pull requests. I did quickly look ahead for layout and I'm still not entirely sure I'm convinced it needs a whole module, but it's fine for now.

@SamWilsn
SamWilsn merged commit 49729e6 into eips-wg:master May 9, 2026
4 checks passed
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.

2 participants