-
Notifications
You must be signed in to change notification settings - Fork 1
[V2:02] Split Preprocessor CLI and Command Modules #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| /* | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
| */ | ||
|
|
||
| //! Changed-file command execution. | ||
|
|
||
| use std::{ | ||
| ffi::OsStr, | ||
| path::{Path, PathBuf}, | ||
| }; | ||
|
|
||
| use snafu::{ResultExt, Whatever}; | ||
|
|
||
| use crate::{cli::ChangedFormat, config::Config, git, layout::REPO_DIR}; | ||
|
|
||
| pub(crate) fn is_proposal_path(mut p: PathBuf) -> bool { | ||
| // Only lint `content/00001.md` and `content/00001/index.md` files. | ||
|
|
||
| // content/00000.md | content/00000/index.md | ||
| // ^^^^^^^^ | ^^^^^^^^ | ||
| match p.file_name() { | ||
| Some(n) if n == "index.md" => { | ||
| p.pop(); | ||
| } | ||
| Some(_) if p.extension().map(|x| x == "md").unwrap_or(false) => { | ||
| p.set_extension(""); | ||
| } | ||
| None | Some(_) => return false, | ||
| } | ||
|
|
||
| // content/00000 | ||
| // ^^^^^ | ||
| match p.file_name().and_then(OsStr::to_str) { | ||
| None => return false, | ||
| Some(f) if f.parse::<u64>().is_err() => return false, | ||
| Some(_) => { | ||
| p.pop(); | ||
| } | ||
| } | ||
|
|
||
| // content | ||
| // ^^^^^^^ | ||
| match p.file_name() { | ||
| Some(f) if f == "content" => { | ||
| p.pop(); | ||
| } | ||
| _ => return false, | ||
| } | ||
|
|
||
| p == OsStr::new("") | ||
| } | ||
|
|
||
| pub(crate) fn run( | ||
| root_path: &Path, | ||
| build_path: &Path, | ||
| config: &Config, | ||
| all: bool, | ||
| format: &ChangedFormat, | ||
| ) -> Result<(), Whatever> { | ||
| let repo_path = build_path.join(REPO_DIR); | ||
|
|
||
| let both = git::Fresh::new(root_path, &repo_path, &config.locations) | ||
| .whatever_context("initializing build repo")? | ||
| .clone_src() | ||
| .whatever_context("cloning source repo")? | ||
| .fetch_upstream() | ||
| .whatever_context("fetching upstream repo")?; | ||
|
|
||
| let changed_files: Vec<_> = both | ||
| .changed_files() | ||
| .whatever_context("unable to list changed files")? | ||
| .into_iter() | ||
| .filter(|p| all || is_proposal_path(p.into())) | ||
| .map(|p| repo_path.join(p)) | ||
| .collect(); | ||
|
|
||
| format.print(&changed_files, &repo_path); | ||
| Ok(()) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| /* | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
| */ | ||
|
|
||
| //! Clap command surface and command helper methods. | ||
|
|
||
| use std::path::{Path, PathBuf}; | ||
|
|
||
| use clap::{Parser, Subcommand}; | ||
|
|
||
| use crate::{lint, print}; | ||
|
|
||
| /// Build script for Ethereum EIPs and ERCs. | ||
| #[derive(Parser, Debug)] | ||
| #[command(version, about)] | ||
| pub(crate) struct Args { | ||
| /// Use ROOT as the base directory (instead of finding it automatically) | ||
| #[clap(short = 'C')] | ||
| pub(crate) root: Option<PathBuf>, | ||
|
|
||
| /// Use the staging repositories (for testing) | ||
| #[clap(long = "staging")] | ||
| pub(crate) staging: bool, | ||
|
|
||
| #[clap(subcommand)] | ||
| pub(crate) operation: Operation, | ||
| } | ||
|
|
||
| #[derive(Debug, Subcommand)] | ||
| pub(crate) enum Operation { | ||
| /// Print various useful things, like available lints | ||
| Print { | ||
| #[command(flatten)] | ||
| print: print::CmdArgs, | ||
| }, | ||
|
|
||
| /// Build the project and output HTML | ||
| Build { | ||
| #[command(flatten)] | ||
| eipw: lint::CmdArgs, | ||
| }, | ||
|
|
||
| /// Build the project and launch a web server to preview it | ||
| Serve { | ||
| #[command(flatten)] | ||
| eipw: lint::CmdArgs, | ||
| }, | ||
|
|
||
| /// Remove temporary and output files | ||
| Clean, | ||
|
|
||
| /// Analyze the repository and report errors, but don't build HTML files | ||
| Check { | ||
| #[command(flatten)] | ||
| eipw: lint::CmdArgs, | ||
| }, | ||
|
|
||
| /// List files changed since the last commit common to both the local and upstream repositories | ||
| Changed { | ||
| /// List all changed files, not just proposals | ||
| #[arg(long, short)] | ||
| all: bool, | ||
| #[clap(long, value_enum, default_value_t)] | ||
| format: ChangedFormat, | ||
| }, | ||
| } | ||
|
|
||
| #[derive(Debug, clap::ValueEnum, Clone, Default)] | ||
| pub(crate) enum ChangedFormat { | ||
| #[default] | ||
| Newline, | ||
| Nul, | ||
| Json, | ||
| } | ||
|
|
||
| impl ChangedFormat { | ||
| fn print_sep(files: &[&Path], sep: &str) { | ||
| let files: Vec<_> = files | ||
| .iter() | ||
| .map(|f| f.to_str().expect("path not UTF-8")) | ||
| .collect(); | ||
| if files.iter().any(|f| f.contains(sep)) { | ||
| panic!("changed file path contains separator"); | ||
| } | ||
| println!("{}", files.join(sep)); | ||
| } | ||
|
|
||
| fn print_json(files: &[&Path]) { | ||
| let stdout = std::io::stdout(); | ||
| serde_json::to_writer_pretty(stdout, files).unwrap(); | ||
| } | ||
|
|
||
| pub(crate) fn print(&self, files: &[PathBuf], repo_path: &Path) { | ||
| let files: Vec<_> = files | ||
| .iter() | ||
| .map(|f| match f.strip_prefix(repo_path) { | ||
| Ok(p) => p, | ||
| _ => f, | ||
| }) | ||
| .collect(); | ||
|
|
||
| match self { | ||
| Self::Newline => Self::print_sep(&files, "\n"), | ||
| Self::Nul => Self::print_sep(&files, "\0"), | ||
| Self::Json => Self::print_json(&files), | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| /* | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
| */ | ||
|
|
||
| use std::path::PathBuf; | ||
|
|
||
| use snafu::{ResultExt, Whatever}; | ||
|
|
||
| use crate::{cli::Args, find_root}; | ||
|
|
||
| pub(crate) fn root(args: &Args) -> Result<PathBuf, Whatever> { | ||
| let dir = match &args.root { | ||
| None => find_root::find_root().whatever_context("cannot find repository root")?, | ||
| Some(p) => p.to_path_buf(), | ||
| }; | ||
| find_root::is_root(&dir).whatever_context("invalid root directory")?; | ||
| Ok(dir) | ||
| } |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file could be a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| /* | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
| */ | ||
|
|
||
| pub(crate) const CONTENT_DIR: &str = "content"; | ||
| pub(crate) const BUILD_DIR: &str = "build"; | ||
| pub(crate) const REPO_DIR: &str = "repo"; | ||
| pub(crate) const OUTPUT_DIR: &str = "output"; |
There was a problem hiding this comment.
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
*_DIRconstants,rootandis_proposal_pathinto one module dedicated to paths. What do you think?There was a problem hiding this comment.
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.rsis 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.rsowns the shared prepared-runtime layout helpers likeoutput_path,mounted_theme_path, andtheme_config_path. Separately,context.rsgrows into CLI/workspace root and input-path resolution.The proposal-path part follows the same pattern. In V2:02,
changed.rscarryingis_proposal_pathis narrower than the eventual ownership boundary. The later stack moves that logic toward the proposal domain rather than a generic paths module:proposal.rsstarts owning proposal-number and proposal-path classification in V2:13, and by V2:18changed.rscallsproposal::is_proposal_pathinstead 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that makes sense. I was questioning whether
is_proposal_pathinchanged.rsmade sense.