Navigation Menu

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 an incorrect usage of strip which caused bad formats to be read #77

Merged
merged 1 commit into from Jul 11, 2016

Conversation

akariv
Copy link
Member

@akariv akariv commented Jul 11, 2016

No description provided.

@zhenyab
Copy link

zhenyab commented Jul 11, 2016

@akariv could you please explain the use case?

@coveralls
Copy link

coveralls commented Jul 11, 2016

Coverage Status

Coverage remained the same at 92.863% when pulling 3a0a490 on akariv:fix/bad-stripping into bfce923 on frictionlessdata:master.

@akariv
Copy link
Member Author

akariv commented Jul 11, 2016

This is a simple bug:

a.strip("fmt:") removes all instances of 'f','m','t,':'from the beginning and the end of the string.

e.g. 'fmt:%y-%m' will be converted to '%y-%' which is not what you'd want here (notice the ending m was removed)

@roll
Copy link
Member

roll commented Jul 11, 2016

@akariv
Thanks! Will you bump version (0.6.5) in PR for quick release (as you did before)?

@vitorbaptista
Copy link
Contributor

vitorbaptista commented Jul 11, 2016

@akariv Interesting catch. Changing strip('fmt:') to lstrip('fmt:') seems to solve it as well, while keeping the code a bit more explicit than [4:].

Edit: Nevermind, lstrip('fmt:') is also not what it looks like. For example, 'fmt:format'.lstrip('fmt:') would return 'ormat', which isn't what we want.

@akariv
Copy link
Member Author

akariv commented Jul 11, 2016

@vitorbaptista not necessarily as fmt:my-format will still be decoded incorrectly (and result in y-format).
@roll done

@coveralls
Copy link

coveralls commented Jul 11, 2016

Coverage Status

Coverage remained the same at 92.863% when pulling 4cb3112 on akariv:fix/bad-stripping into bfce923 on frictionlessdata:master.

@@ -69,7 +69,7 @@ def __init__(self, field=None):
self.__format_fmt = None
if self.__format.startswith('fmt'):
Copy link
Member

Choose a reason for hiding this comment

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

@akariv
I suppose here we also need fix fmt: instead of fmt - to avoid things like fmtX%y-%m

Copy link
Member Author

Choose a reason for hiding this comment

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

True. Fixed.

@coveralls
Copy link

coveralls commented Jul 11, 2016

Coverage Status

Coverage remained the same at 92.863% when pulling 4cb3112 on akariv:fix/bad-stripping into bfce923 on frictionlessdata:master.

@coveralls
Copy link

coveralls commented Jul 11, 2016

Coverage Status

Coverage remained the same at 92.863% when pulling c3020e4 on akariv:fix/bad-stripping into bfce923 on frictionlessdata:master.

@roll roll merged commit 977c9c0 into frictionlessdata:master Jul 11, 2016
@pwalsh
Copy link
Member

pwalsh commented Jul 11, 2016

more to the point, we need to remove fmt: from the spec: frictionlessdata/specs#260

@roll
Copy link
Member

roll commented Jul 11, 2016

@pwalsh
I know but we're waiting spec for now to make changes here right?

@roll
Copy link
Member

roll commented Jul 11, 2016

@akariv
released as 0.6.5 - https://pypi.python.org/pypi/jsontableschema

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

Successfully merging this pull request may close these issues.

None yet

6 participants