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

Option handling improvements #2991

Merged
merged 5 commits into from
Feb 12, 2019
Merged

Option handling improvements #2991

merged 5 commits into from
Feb 12, 2019

Conversation

codexnull
Copy link
Contributor

@codexnull codexnull commented Feb 8, 2019

This change is Reviewable

Javier Alvarado added 3 commits February 7, 2019 11:44
@codexnull codexnull requested a review from a team February 8, 2019 01:22
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @codexnull)


dgraph/cmd/live/run.go, line 351 at r1 (raw file):

	if opt.dataFiles == "" {
		return errors.New("RDF or JSON file(s) location must be specified\n")

instances of type error should not have a new line at the end of the message.


dgraph/cmd/live/run.go, line 357 at r1 (raw file):

	totalFiles := len(filesList)
	if totalFiles == 0 {
		return fmt.Errorf("No data files found in %s\n", opt.dataFiles)

No new line here as well

Copy link
Contributor Author

@codexnull codexnull left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @martinmr)


dgraph/cmd/live/run.go, line 351 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

instances of type error should not have a new line at the end of the message.

Done.


dgraph/cmd/live/run.go, line 357 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

No new line here as well

Done.

Copy link

@gitlw gitlw left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions (waiting on @codexnull and @martinmr)


dgraph/cmd/live/run.go, line 187 at r2 (raw file):

				loadType = chunker.JsonFormat
			} else {
				return fmt.Errorf("Need --format=rdf or --format=json to load %s", filename)

Minor: lower case error strings https://github.com/golang/go/wiki/CodeReviewComments#error-strings
Otherwise LGTM

Copy link

@gitlw gitlw left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions (waiting on @codexnull and @martinmr)

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions (waiting on @codexnull and @martinmr)


dgraph/cmd/live/run.go, line 87 at r3 (raw file):

	flag.StringP("files", "f", "", "Location of *.rdf(.gz) or *.json(.gz) file(s) to load")
	flag.StringP("schema", "s", "", "Location of schema file")
	flag.String("format", "", "Specify file format (rdf or json) instead of getting it from filename")

100 chars.

@codexnull codexnull merged commit 87c3674 into master Feb 12, 2019
@codexnull codexnull deleted the javier/option_handling branch March 1, 2019 22:53
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
* A user should not be faced with a stack trace due to a mistyped command line option. Don't output error message twice.

* Don't output usage on invalid command line option since it can bury the error message.

* Add --format option to bulk and live loaders. Replace --rdfs and --jsons in bulk loader with --files to match live loader.

* Fix errant newlines in error messages.

* Fix error message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants