-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add coloring and style to create interactive command #56
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
base: master
Are you sure you want to change the base?
Conversation
21ac553 to
e18cf85
Compare
|
@mostafa630 Thanks Mostafa! I tried it locally and It did panic for some reason. I'll check it out later today. fahd ~/metassr$ ./target/release/metassr create
> Project name: new project
> Template: javascript
> Version: 1.0.0
> Description: A web application built with MetaSSR framework
thread 'main' (29660) panicked at crates/metassr-create/src/templates.rs:15:18:
internal error: entered unreachable code: Template isn't detected.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace |
e18cf85 to
1067a95
Compare
|
@fahdfady Issue: The Solution: I separated the colored display from the plain text conversion. I added a method that returns the clean text without any ANSI codes, which is used for internal operations, while still keeping the colored output for the terminal display. Now everything works perfectly!
|
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.
LGTM.
I'm still not sure if this is fine here https://github.com/metacall/metassr/pull/56/files#diff-cd7417aa4a345af41213bbe063f15a4f9294af9342f8e936913c56ce5f1667bbR104-R108 but I think @hulxv will give a better review than mine.
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.
Pull Request Overview
This PR adds template-specific coloring to the CLI by introducing a color system for each template variant. The main goal is to make the template display more visually distinct while maintaining extensibility for future templates.
- Introduced a
data()method to centralize template metadata (display text and color) - Added a
DEFAULT_COLORconstant for easy color management across templates - Refactored the
Displayimplementation to use colored output via crossterm
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| metassr-cli/src/cli/creator.rs | Adds template coloring system with data() and as_str() methods, updates Display to use colored output |
| Cargo.toml | Adds crossterm dependency for terminal color support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Looks fine, but we need to do a few little things
| use tracing::{error, info}; | ||
|
|
||
| use super::traits::Exec; | ||
| pub const DEFAULT_COLOR: Color = Color::White; |
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.
It's unused. Why do we need this?
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.
@hulxv
You’re absolutely right, but I included it here because in the future, if we add more templates, we can assign a default color to each one and use it directly.
| metassr-bundler = { path = "crates/metassr-bundler" } | ||
| metassr-fs-analyzer = { path = "crates/metassr-fs-analyzer" } | ||
| metassr-watcher = { path = "crates/metassr-watcher" } | ||
| crossterm = "0.29.0" |
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 see you use crossterm only for colors. I think it's a bad idea. We can use these colors directly without adding this big boy.
We can do something like this
pub const RESET: &str = "\x1b[0m";
pub const YELLOW: &str = "\x1b[93m";
pub const BLUE: &str = "\x1b[94m";
// ....
impl Template {
fn fmt_colored(&self) -> String {
let (text, color) = self.data();
format!("{}{}{RESET}", color, text)
}
}| Self::Typescript => "typescript", | ||
| }) | ||
| let (text, color) = self.data(); | ||
| write!(f, "{}", text.with(color)) |
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 putting this inside the Display implementation is a bad idea. make a method for that instead
| } | ||
|
|
||
| impl Template { | ||
| pub fn data(&self) -> (&'static str, Color) { |
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.
Instead of that, we can make a HashMap that contains the template name and its color. This will be simpler than making two functions. 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.
@hulxv
Do you mean using a global HashMap that maps each template to its corresponding color?
|
@mostafa630 You can use a keyword in the PR description to link the PR to the issue. |


Added a
data()method to each Template variant to provide its display text and color, so thefmt()implementation doesn’t need to change even when new templates are added.We also use a
DEFAULT_COLORconstant for future templates, making it easy to assign a default color. If several templates share this default, you can simply change DEFAULT_COLOR once, and it will update for all of them.Close #53