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

Fix parsing of git urls with windows volumes in them #631

Merged
merged 1 commit into from
Jan 19, 2018

Conversation

lox
Copy link
Contributor

@lox lox commented Jan 19, 2018

This change handles parsing urls like file:///C:/Users/vagrant/repo.

@lox lox requested a review from keithpitt January 19, 2018 03:59
@lox
Copy link
Contributor Author

lox commented Jan 19, 2018

This was a particularly gross usage of table tests. I think the result post discussion in #630 tells a much better story. Feels @keithpitt?

{"ssh://git@scm.xxx:7999/yyy/zzz.git", "ssh://git@scm.xxx:7999/yyy/zzz.git", false},
{"ssh://root@git.host.de:4019/var/cache/git/project.git", "ssh://root@git.host.de:4019/var/cache/git/project.git", false},
u, err := parseGittableURL(`/home/vagrant/repo`)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be...

 assert.Nil(err)

..instead?

Copy link
Member

Choose a reason for hiding this comment

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

I also learnt about this syntax:

func TestSomething(t *testing.T) {
  assert := assert.New(t)

  assert.Nil(err)
}

So you don't need to pass t around everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with that is that assert.Nil isn't actually fatal, it doesn't stop the test. I tend to think the if block with a Fail is a lot clearer here. Go's error handling is meant to look like this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The alternative would be:

if !assert.NoError(err) {
  t.FailNow()
}

Which is ok I guess? Doesn't seem worth the cognitive overhead.

Copy link
Member

Choose a reason for hiding this comment

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

Orrr....

import (
  "github.com/stretchr/testify/require"
)

func TestSomething(t *testing.T) {
  require := require.New(t)

  require.Nil(err)
}

That'll blow up the test (instead of running) if it fails! Maybe better/worse?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes me sad :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reckon I'm going to stick with this as is rather than bring in a whole new package.

Copy link
Member

Choose a reason for hiding this comment

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

Can you just use require instead of assert for the whole thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, we could. I guess if we are doing the whole testify thing we might as well bring it all in.

@lox lox merged commit e6456bd into master Jan 19, 2018
@lox lox deleted the fix-gittable-parsing-on-windows branch January 19, 2018 05:01
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

2 participants