-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
feat(cli): add --no-clear-screen
flag
#13454
Conversation
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.
An integration test would be useful for this flag. See cli/tests/integration/watcher_tests.rs
for example tests.
Added one integration test |
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-yu nice work! I got a few nitpicks but otherwise LGTM
cli/flags.rs
Outdated
@@ -1768,6 +1781,8 @@ fn bundle_parse(flags: &mut Flags, matches: &clap::ArgMatches) { | |||
|
|||
watch_arg_parse(flags, matches, false); | |||
|
|||
no_clear_screen_arg_parse(flags, matches); |
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.
Maybe merge this function with watch_arg_parse
?
#[test] | ||
fn test_watch_with_no_clear_screen() { |
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 good 👍 please add a test case that uses --no-clear-screen
but doesn't use --watch
and assert that it returns error
cli/main.rs
Outdated
|
||
tools::lint::lint(maybe_lint_config, lint_flags, flags.watch.is_some()) | ||
.await?; | ||
tools::lint::lint(lint_flags, flags).await?; |
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.
Please flip order of the arguments (flags, lint_flags
)
cli/tools/lint.rs
Outdated
lint_flags: LintFlags, | ||
watch: bool, | ||
) -> Result<(), AnyError> { | ||
pub async fn lint(lint_flags: LintFlags, flags: Flags) -> Result<(), AnyError> { |
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.
Ditto
@bartlomieju Update |
9a58145
to
5705384
Compare
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, @dsherret PTAL too
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, thank you @ah-yu
Closes #13315