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

Add back an invalid url to the invalid_urls() fixture #18

Merged
merged 1 commit into from
Nov 3, 2018
Merged

Add back an invalid url to the invalid_urls() fixture #18

merged 1 commit into from
Nov 3, 2018

Conversation

jameshiew
Copy link
Contributor

This restores some coverage

test/conftest.py Outdated
@@ -263,4 +263,4 @@ def git_urls():

@pytest.fixture()
def invalid_urls():
return []
return ['not a valid URL']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this a url, one that the regex does not match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't think of one which might not be supported. May be better to rename this fixture to invalid_strings and the test to something like test_parse_raises_on_invalid_string?

Copy link
Member

@jayvdb jayvdb Oct 23, 2018

Choose a reason for hiding this comment

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

ya, renaming it sounds good
(and change the commit message)

@retr0h
Copy link
Collaborator

retr0h commented Oct 25, 2018

rad @jameshiew, looks like travis is failing.

@retr0h
Copy link
Collaborator

retr0h commented Oct 25, 2018

rad @jameshiew, looks like travis is failing.

Oh looks like it's an issue with flake8 3.6.

@retr0h
Copy link
Collaborator

retr0h commented Oct 25, 2018

#19 fixes the issue, please rebase against master.

@retr0h retr0h merged commit 16c4cef into coala:master Nov 3, 2018
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.

3 participants