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

Improve error output for missing or malformed '-database' and '-source' parameters #88

Merged
merged 1 commit into from Aug 8, 2018

Conversation

HaraldNordgren
Copy link
Contributor

@HaraldNordgren HaraldNordgren commented Aug 5, 2018

Related to #89

The current output only reads "no scheme" which is not really helpful.

Updates mattes/migrate#270
Updates mattes/migrate#317

@coveralls
Copy link

coveralls commented Aug 5, 2018

Pull Request Test Coverage Report for Build 164

  • 17 of 17 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+3.02%) to 49.161%

Totals Coverage Status
Change from base Build 160: 3.02%
Covered Lines: 850
Relevant Lines: 1729

💛 - Coveralls

@HaraldNordgren HaraldNordgren force-pushed the error_output branch 2 times, most recently from 6768262 to b2ab621 Compare August 5, 2018 20:50
@HaraldNordgren HaraldNordgren changed the title Document that '-source' is a required parameter and improve error output Improve error output for missing or malformed '-database' and '-source' parameters Aug 5, 2018
@HaraldNordgren
Copy link
Contributor Author

Merge this @dhui @mattes?

@dhui
Copy link
Member

dhui commented Aug 7, 2018

I'm not sure if this is the correct place to validate parameters.
Do you have any examples of empty urls being used with migrate as a library?

If all of the confusion is with the migrate CLI, then we only need to validate the parameters in the CLI. e.g. ensure that source/path is specified

@HaraldNordgren
Copy link
Contributor Author

HaraldNordgren commented Aug 7, 2018

I'm not sure. I haven't used the as a library yet. But I would imagine that usage like in godoc_vfs/vfs_example_test.go where we call

m, err := migrate.NewWithSourceInstance("godoc-vfs", d, "database://foobar")
...

would suffer from the same problems if database://foobar was replaced with something like os.Env("DATABASE_PATH") and that env variable ends up being empty for any reason. I'm making an assumption since I haven't tried this myself. But I assume that used when as a library it would still suffer from the same problems of unclear error messages.

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

env variable ends up being empty for any reason

Good point. Let's validate the URLs then.

util.go Outdated
@@ -44,14 +44,34 @@ func suint(n int) uint {
}

var errNoScheme = fmt.Errorf("no scheme")
var errEmpty = fmt.Errorf("cannot be empty")
Copy link
Member

Choose a reason for hiding this comment

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

Rename to errEmptyURL and use the string "URL cannot be empty".

Also, use errors.New() instead of fmt.Errorf() if string formatting isn't used.

util_test.go Outdated
cases := []struct {
name string
given string
expectErr error
Copy link
Member

Choose a reason for hiding this comment

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

Re-structure this to test valid URLs as well

util_test.go Outdated
func TestSourceSchemeFromUrl(t *testing.T) {
cases := []struct {
name string
given string
Copy link
Member

Choose a reason for hiding this comment

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

rename to urlStr

@HaraldNordgren HaraldNordgren force-pushed the error_output branch 3 times, most recently from 37e8240 to d22e0c4 Compare August 8, 2018 19:24
@HaraldNordgren
Copy link
Contributor Author

@dhui Updated.

@dhui dhui merged commit 6092802 into golang-migrate:master Aug 8, 2018
@HaraldNordgren HaraldNordgren deleted the error_output branch August 9, 2018 07:12
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.

None yet

3 participants