Skip to content

Conversation

RonnyPfannschmidt
Copy link
Contributor

No description provided.

Copy link
Contributor

@anarcat anarcat left a comment

Choose a reason for hiding this comment

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

looks good, but i'm wondering if the test suite would find a similar regression in the future.



def dump_version(root, version, write_to, template=None):
assert isinstance(version, string_types)
Copy link
Contributor

Choose a reason for hiding this comment

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

does this detect the new failure?

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 code in dump-version only works with strings, perhaps even this check is not trough enough

local_scheme=local_scheme)
return version

return version_string
Copy link
Contributor

Choose a reason for hiding this comment

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

i like this: much clearer than the previous implementation - i thought of doing something like that in my own PR but refrained to keep things simple...

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 did a more intensive restructure ensuring i never get a string there - the shortcut was an premature optimization, and as such the root of the evil ^^

def test_dump_version_doesnt_bail_on_value_error(tmpdir):
write_to = "VERSION"
version = VERSIONS['exact']
version = str(VERSIONS['exact'].tag)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what isthis for - would this silence possible unit test failures that could detect the fixed bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exact.tag is a setultools version not a string

@RonnyPfannschmidt
Copy link
Contributor Author

@anarcat @untitaker unless you have further comments i'm about to release 1.13

@untitaker
Copy link
Contributor

lgtm

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