-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Replace --quiet with --banner #23343
Replace --quiet with --banner #23343
Conversation
Awesome! Maybe we should deprecate |
base/client.jl
Outdated
@@ -253,7 +253,7 @@ function process_options(opts::JLOptions) | |||
repl = true | |||
startup = (opts.startupfile != 2) | |||
history_file = (opts.historyfile != 0) | |||
quiet = (opts.quiet != 0) | |||
banner = (opts.banner == 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.
This should read as either banner = (opts.banner != 0)
or quiet = (opts.banner == 0)
.
@@ -315,6 +314,18 @@ JL_DLLEXPORT void jl_parse_opts(int *argcp, char ***argvp) | |||
jl_options.image_file = strdup(optarg); | |||
jl_options.image_file_specified = 1; | |||
break; | |||
case 'q': // quiet | |||
jl_printf(JL_STDERR, "-q and --quiet are deprecated, use --banner=no instead\n"); |
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.
should add a comment in the 0.7 section of base/deprecated.jl so this gets staged for removal at the right time
also should add a NEWS.md note about the changed command-line flag
I'm not sure why |
It's a known problem (#23215). Restarted :) |
Now the Travis build exceeded the maximum build time and the CircleCI x86 build has encountered a strange issue I can't recreate locally with |
if term.term_type == "dumb" | ||
active_repl = REPL.BasicREPL(term) | ||
quiet || warn("Terminal not fully functional") | ||
banner && warn("Terminal not fully functional") |
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 a bit strange to depend on banner
for this warning.
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.
True, but I would argue that's beyond the scope of this PR. It's just that the better option name made it clear how weird it is that this works this way, so that's a good sign for this change. I've opened a separate issue about this: #23380.
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.
quiet
could be (in the future) an umbrella option for a bag of others, including banner
and supressing this warning above; but until there are more candidates to be controlled by quiet
, it seems fine to have banner control this warning.
NEWS.md
Outdated
@@ -946,6 +946,8 @@ Command-line option changes | |||
ignored.) This flag is also available in non-`polly` builds (`USE_POLLY := 0`), | |||
but has no effect ([#18159]). | |||
|
|||
* The `-q` and `--quiet` flags have been deprecated in favor of `--banner={yes|no}` ([#23342]). |
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.
This should be in the 0.7 section instead of 0.6, around line 320 in this file :)
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.
Great PR, @HarrisonGrodin – thank you!
Fixes #23342.