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

schema: modify Makefile #750

Merged
merged 2 commits into from
Apr 10, 2017
Merged

schema: modify Makefile #750

merged 2 commits into from
Apr 10, 2017

Conversation

zhouhao3
Copy link

Signed-off-by: zhouhao [email protected]

@hqhq
Copy link
Contributor

hqhq commented Apr 5, 2017

I don't think it's necessary and it'll make the help information non-accurate.

Rejected.

Rejected with PullApprove

@vbatts
Copy link
Member

vbatts commented Apr 5, 2017

I don't see that it makes the help info incorrect. I'm two-ways about the default make target being help. Honestly, most of the time I expect the default target to be sensible, like building the tool or running the tests.
This PR LGTM in that sense

@hqhq
Copy link
Contributor

hqhq commented Apr 6, 2017

@vbatts Not that incorrect, just not completely accurate.

$ make
Usage: make <target>

 * 'fmt' - format the json with indentation
 * 'validate' - build the validation tool

To me it means "make needs an target, which can be fmt or validate", if we make validate default and target optional, I would like to see something like:

$ make help
Usage: make [target]

 * 'fmt' - format the json with indentation
 * 'validate' - build the validation tool (default)
 * 'help' - show this help information

I'm also fine with changing the default target to be running the tests, just needs to fix help information as well.

@vbatts
Copy link
Member

vbatts commented Apr 6, 2017

@hqhq oh oh, good catch. so yes, using square brackets there would be more accurate 😸

@zhouhao3
Copy link
Author

zhouhao3 commented Apr 10, 2017

@hqhq Thanks for your advice, updated, PTAL.

@hqhq
Copy link
Contributor

hqhq commented Apr 10, 2017

LGTM

Approved with PullApprove

@zhouhao3
Copy link
Author

zhouhao3 commented Apr 10, 2017

@vbatts @mrunalp PTAL

@vbatts
Copy link
Member

vbatts commented Apr 10, 2017

LGTM

Approved with PullApprove

@vbatts vbatts merged commit daba39c into opencontainers:master Apr 10, 2017
@zhouhao3 zhouhao3 deleted the schema-test branch April 11, 2017 01:32
@vbatts vbatts mentioned this pull request Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants