Conversation
matbrgz
left a comment
There was a problem hiding this comment.
Revisão — não mergear como está. O plano e o completions são bons; o problema é que o PR publica na superfície do CLI oito subcomandos que não fazem nada, e um deles mente dizendo que fez.
1. config set reporta sucesso sem gravar nada
ConfigAction::Set { key, value } => {
println!("Config set: {} = {}", key, value);
// TODO implement writing back to config file
}O usuário roda rustyll config set url https://meusite.com, lê "Config set: url = https://meusite.com" e sai convencido de que a configuração mudou. Nada foi escrito. Em seguida ele roda o build, não entende por que a URL está errada, e não tem motivo nenhum para suspeitar do comando que acabou de confirmar a operação.
Esse é o item mais sério do PR, porque não é uma feature faltando — é uma feature que relata algo falso. Enquanto não gravar de verdade, tem que ser erro explícito:
ConfigAction::Set { .. } => {
eprintln!("error: `config set` ainda não foi implementado");
std::process::exit(1);
}2. theme e plugin são seis println! e nada mais
ThemeAction::Install { name_or_url } => { println!("Theme install {}", name_or_url); }
ThemeAction::List {} => { println!("Theme list"); }
ThemeAction::Apply { name } => { println!("Theme apply {}", name); }
PluginAction::Install { name } => { println!("Plugin install {}", name); }
PluginAction::List {} => { println!("Plugin list"); }
PluginAction::Enable { name } => { println!("Plugin enable {}", name); }Como estão registrados no enum Commands com doc comments (/// Manage themes, /// Manage plugins), o rustyll --help passa a anunciar gerenciamento de temas e plugins. É promessa que o binário não cumpre, e vira issue de usuário.
Três saídas, em ordem de preferência: (a) deixar fora deste PR e trazer quando tiverem implementação; (b) manter o enum mas marcar #[command(hide = true)] enquanto não funcionam; (c) manter visível, mas sair com código de erro e mensagem de "não implementado" — nunca uma mensagem que pareça saída normal.
3. rustyll migrate quebra para quem já usa — e o PR não menciona
- Migrate { source, destination, engine, verbose, clean },
+ #[command(name = "migrate", alias = "m", subcommand)]
+ Migrate(MigrateCommands),
+
+pub enum MigrateCommands {
+ Run { source, destination, engine, verbose, clean },
+ ListPlatforms {},Na prática, rustyll migrate -e jekyll -s ./site deixa de funcionar e passa a exigir rustyll migrate run -e jekyll -s ./site. É uma mudança incompatível na interface pública do CLI, e o corpo do PR ("draft plan for refactoring", "add completion script generator command") não a cita.
Se for intencional, precisa: constar na descrição, entrar no CHANGELOG, e idealmente manter migrate sem subcomando funcionando como alias de migrate run por um ciclo de depreciação. Se não for intencional, dá para expor ListPlatforms como uma flag (migrate --list-platforms) e preservar a forma antiga.
4. cache clear apaga caminhos relativos ao diretório atual, sem confirmação
const CACHE_DIRS: &[&str] = &[".jekyll-cache", ".sass-cache"];
const CACHE_FILES: &[&str] = &[".jekyll-metadata"];Nenhuma checagem de que o diretório corrente é a raiz de um site (um _config.yml, por exemplo). Rodado no lugar errado, o comando remove silenciosamente o que encontrar com esses nomes. Para um comando destrutivo, vale: verificar a raiz do site, listar o que será apagado, e exigir --yes ou confirmação interativa.
Detalhe relacionado:
Some(_) => println!("Unknown cache type"),Tipo inválido imprime no stdout e sai com código 0 — um script que encadeia comandos não tem como detectar o erro. Isso deveria ser um value_enum do clap (assim o próprio clap valida e lista as opções válidas no --help), ou no mínimo eprintln! + saída diferente de zero.
5. Erros engolidos em silêncio
let yaml = serde_yaml::to_value(&cfg).unwrap_or(Value::Null);
println!("{}", serde_yaml::to_string(v).unwrap_or_default());unwrap_or(Value::Null) e unwrap_or_default() transformam falha de serialização em saída vazia e código de retorno 0. E if let Ok(cfg) = ... else println!("Failed to load configuration") descarta o erro real — o usuário não fica sabendo se o _config.yml não existe, está mal formado ou não pôde ser lido.
6. PathBuf::from(".") fixo ignora o resto do CLI
config get e config list carregam a config sempre do diretório corrente. Se o CLI tem (ou vai ter) um --source/-s global, esses dois comandos não o respeitam.
7. Dois --verbose independentes
O PR adiciona verbose global na struct Cli, e MigrateCommands::Run mantém o seu próprio -v/--verbose. Ficam dois flags com o mesmo nome em níveis diferentes, sem relação entre si — rustyll --verbose migrate run e rustyll migrate run --verbose fazem coisas diferentes. Vale consolidar no global.
8. Dois arquivos TODO.md
Este PR cria design/TODO.md; a #1 criou TODO.md na raiz. Duas listas de pendências divergem em uma semana. Vale escolher uma.
9. cargo check não é teste
A seção Testing diz só cargo check. Isso confirma que compila — e é exatamente o tipo de validação que não percebe que metade dos comandos novos só imprime texto. Nem cargo test foi rodado.
O que está bom
completions.rsé o melhor pedaço do PR: usaCommandFactorycorretamente, deriva do próprioCli(então nunca sai de sincronia com os comandos reais) e escreve emstdout, que é o esperado para gerar script de completion.clap_complete = "4.4"casa com a versão do clap.get_nested_valuecomkey.split('.')é uma implementação limpa de acesso aninhado (rustyll config get site.title), e retornaNonecorretamente ao esbarrar num não-mapping.migrate list-platformsusandomigrate::list_engines()é útil e está de fato implementado.design/CLI_REFACTOR_PLAN.mdcom 99 linhas de plano é justamente o tipo de documento que faltava — ele é a parte do PR que eu mergearia hoje.
Sugestão
Fatiar em três: (1) o plano + completions + migrate list-platforms — pronto para merge; (2) config get/list com tratamento de erro de verdade e respeitando --source; (3) cache, theme, plugin e config set, quando tiverem implementação. A mudança incompatível do migrate (item 3) precisa de decisão explícita em qualquer cenário.
Summary
Testing
cargo checkhttps://chatgpt.com/codex/tasks/task_e_684a6c5032f883268caa4295825e2e3a