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

Use custom ParserError class in parser exceptions #881

Merged
merged 4 commits into from Apr 23, 2019
Merged

Use custom ParserError class in parser exceptions #881

merged 4 commits into from Apr 23, 2019

Conversation

gfyoung
Copy link
Contributor

@gfyoung gfyoung commented Feb 25, 2019

If multiple arguments are passed in, it raises a tuple, not one string.

xref pandas-dev/pandas#25430 (comment)

@pganssle
Copy link
Member

There was a similar issue in setuptools recently and I realized that someone might be actually using the args parameter here to more cleanly pull out the timestr.

@jbrockmendel Do you remember what happened with the plan to switch these ValueErrors over to a custom Parser error class? I'm thinking maybe a better solution would be something like this:

class ParserError(ValueError):
    def __repr__(self):
        if len(self.args) > 1:
            return self.__class__.name + ": " + (self.args[0] % self.args[1:])
        else:
            return super(ParserError, self).__repr__()

Then we can have our cake and eat it, too.

@jbrockmendel
Copy link
Contributor

Do you remember what happened with the plan to switch these ValueErrors over to a custom Parser error class?

Nothing comes to mind. I've been going through older issues/PRs, will mention if I come across it

@gfyoung
Copy link
Contributor Author

gfyoung commented Mar 14, 2019

@pganssle @jbrockmendel : Friendly ping here. Any more thoughts on this one?

@jbrockmendel
Copy link
Contributor

LGTM. @pganssle go for it?

@pganssle
Copy link
Member

@jbrockmendel I keep going back and forth on this because I do think it might be useful for someone to be able to extract the timestr simply with .args[1], and someone may be counting on that.

I'm leaning more towards the solution of switching these from ValueError to ParserError, where ParserError is defined as above. Not sure if it should be __repr__ or __str__, though.

@gfyoung
Copy link
Contributor Author

gfyoung commented Mar 17, 2019

@pganssle : Should the error be defined in the same file, or is there a better place for it?

I keep going back and forth on this because I do think it might be useful for someone to be able to extract the timestr simply with .args[1], and someone may be counting on that.

From the perspective of a maintainer, I find this argument a little nebulous for using ParserError, but as a first-time contributor to this codebase, fair enough 🙂

@pganssle
Copy link
Member

@gfyoung Well, the thing is I generally don't want to make any guarantees about the contents of an error message, which is why I don't generally test with any sort of regex match on the errors. The reason is that I don't consider that format part of the public interface.

I would be more comfortable with the idea that ValueError(msg, timestr) could have the "timestr" part of the exception as something that is part of the interface, because it's something that is cleanly accessible by some code that attempts to recover from the error. I'm not sure I was deliberately trying to make that happen, but the original contributor may have been and people may be relying on it now.

Given that there is a way to both make the error look nice when printed and make timestr easily accessible without parsing the error message, I'm inclined to do so rather than risk having to quibble about what is and is not considered a backwards-incompatible change.

@pganssle
Copy link
Member

@gfyoung Ah, missed the question, I think same file is best, and you can add it to __all__.

@gfyoung
Copy link
Contributor Author

gfyoung commented Mar 18, 2019

@pganssle : I had to make some adjustments to your suggested ParserError implementation because it wasn't rendering properly when put raise in front of it. However, it should be good now.

PTAL.

@gfyoung
Copy link
Contributor Author

gfyoung commented Apr 2, 2019

@pganssle @jbrockmendel : Friendly ping here. Any more thoughts on this one?

@gfyoung
Copy link
Contributor Author

gfyoung commented Apr 18, 2019

@pganssle @jbrockmendel : Another friendly ping here. Any more thoughts on this one?

@jbrockmendel
Copy link
Contributor

@pganssle call the ball

Copy link
Member

@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

I'm very sorry that I've been remiss about merging this. I have noted two minor changes that need to be made, but I have done this primarily for my own notes, since I can simply make the required changes myself as part of the merge.

I can update and merge this tomorrow or the next day. Feel free to ping again if I don't have it done by Tuesday.

dateutil/parser/_parser.py Outdated Show resolved Hide resolved
changelog.d/881.bugfix.rst Outdated Show resolved Hide resolved
@gfyoung
Copy link
Contributor Author

gfyoung commented Apr 20, 2019

I'm very sorry that I've been remiss about merging this.

No worries. I'm no stranger to the time crunch that maintainers can have 🙂

I have noted two minor changes that need to be made, but I have done this primarily for my own notes...I can update and merge this tomorrow or the next day.

Sounds good. I'll ping on Tuesday if necessary. Thanks!

Copy link
Member

@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

OK, I made the required changes, added a test and then changed the tests to check against ParserError rather than ValueError.

@pganssle
Copy link
Member

Hm.. Build failure looks unrelated. We can merge this one after the tz master build is fixed.

gfyoung and others added 4 commits April 23, 2019 10:50
This gives a nicer string representation while retaining access to the
input string in the exception arguments.
Ensures that ValueErrors raised when building the datetime from an
already-parsed string will be raised as ParserError as well.
ParserError is a subclass of ValueError, so this just makes the tests
stricter, and this can be considered a backwards-compatible change.
@pganssle pganssle changed the title Correct ValueError message raising Use custom ParserError class in parser exceptions Apr 23, 2019
@pganssle pganssle merged commit 833daf0 into dateutil:master Apr 23, 2019
@pganssle pganssle mentioned this pull request Nov 3, 2019
@gfyoung gfyoung deleted the value-error-messages branch November 3, 2019 04:13
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

3 participants