Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(task): remove --no-config as task subcommand argument #14983

Merged
merged 6 commits into from
Jun 28, 2022
Merged

fix(task): remove --no-config as task subcommand argument #14983

merged 6 commits into from
Jun 28, 2022

Conversation

GJZwiers
Copy link
Contributor

Fixes #14971.

cli/flags.rs Outdated
Comment on lines 2097 to 2104
Arg::new("config")
.short('c')
.long("config")
.value_name("FILE")
.help("Specify the configuration file")
.long_help(CONFIG_HELP.as_str())
.takes_value(true)
.value_hint(ValueHint::FilePath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider splitting config_args() into config_arg and no_config_arg and applying them to all relevant subcommands. I think this is better than duplicating arg definition

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, but besides LGTM

@@ -2997,6 +3003,14 @@ fn config_args_parse(flags: &mut Flags, matches: &ArgMatches) {
};
}

fn task_args_parse(flags: &mut Flags, matches: &ArgMatches) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please inline contents of this function into task_parse, there's no need for a separate function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

@GJZwiers GJZwiers marked this pull request as ready for review June 28, 2022 18:03
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@bartlomieju bartlomieju merged commit 5b7bcef into denoland:main Jun 28, 2022
@GJZwiers GJZwiers deleted the fix_task_remove_no_config_arg branch June 28, 2022 20:01
dsherret pushed a commit to dsherret/deno that referenced this pull request Jun 30, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No apparent use case for deno task --no-config
2 participants