Skip to content

PythonParser::_check_thousands appears broken #4596

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

Closed
cancan101 opened this issue Aug 18, 2013 · 1 comment · Fixed by #4602
Closed

PythonParser::_check_thousands appears broken #4596

cancan101 opened this issue Aug 18, 2013 · 1 comment · Fixed by #4602
Labels
Bug IO CSV read_csv, to_csv IO Data IO issues that don't fit into a more specific label
Milestone

Comments

@cancan101
Copy link
Contributor

This code appears broken:

    def _check_thousands(self, lines):
        if self.thousands is None:
            return lines
        nonnum = re.compile('[^-^0-9^%s^.]+' % self.thousands)
        ret = []
        for l in lines:
            rl = []
            for x in l:
                if (not isinstance(x, compat.string_types) or
                    self.thousands not in x or
                        nonnum.search(x.strip())):
                    rl.append(x)
                else:
                    rl.append(x.replace(',', ''))
            ret.append(rl)
        return ret

It looks like the thousands argument to the class is used to check if the value is "non numeric" but then a hard coded comma is used when actually performing the cleaning.

In addition to fixing this, I would recommend factoring out this method so that it can be used elsewhere.

@cpcloud
Copy link
Member

cpcloud commented Aug 18, 2013

Can you show a small test case where this isn't working? At that point you'd be halfway to a PR 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug IO CSV read_csv, to_csv IO Data IO issues that don't fit into a more specific label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants