Skip to content
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

command/validate: Check that all variables are specified #13872

Merged
merged 2 commits into from
Jul 5, 2017

Conversation

radeksimko
Copy link
Member

Closes #8588

This is a rebased #8588. All the hard work is still credited to @dtolnay - I just patched his PR to comply with the recent changes to context.

This is introducing BC in the sense that default behaviour is changing, but previous behaviour is still available under a flag.

Either way we should mark this as BC in Changelog and also I'd like someone to review this patched PR before I merge it.

@radeksimko radeksimko force-pushed the f-validate-var-check branch from ad623ec to afb729d Compare June 22, 2017 18:31
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

Sorry, this one fell of my radar a bit... implementation-wise this looks fine, and I just have one bit of UX feedback inline.


Options:

-config-only If specified, the command will check basic syntax of
Copy link
Contributor

Choose a reason for hiding this comment

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

This flag name feels like it could hurt in the long run if validate were to get other tweakables like this.

Perhaps this could be -check-variables=false, with the default being -check-variables=true, and then if we add other things in the future they could be like -check-types=false or whatever, defaulting to everything being on by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea, I'll change that to -check-variables=[true]

@radeksimko radeksimko force-pushed the f-validate-var-check branch from 418a5fa to f694871 Compare July 3, 2017 17:00
@radeksimko
Copy link
Member Author

@apparentlymart Fixed.

The command-line flags are all optional. The available flags are:

* `-config-only` - If specified, the command will check basic syntax of the
Copy link
Contributor

Choose a reason for hiding this comment

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

This doc is lagging behind the changes to the interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doh, I did grep through the code, but only in command/*, will fix that in a minute.

@apparentlymart apparentlymart added cli and removed core labels Jul 5, 2017
@radeksimko radeksimko force-pushed the f-validate-var-check branch from f694871 to 2873b2f Compare July 5, 2017 16:17
@radeksimko radeksimko merged commit 14614a5 into master Jul 5, 2017
@radeksimko radeksimko deleted the f-validate-var-check branch July 5, 2017 16:32
@radeksimko radeksimko changed the title command/validate: Add flag to check that all variables are specified command/validate: Check that all variables are specified Jul 5, 2017
@ghost
Copy link

ghost commented Apr 8, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants