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.

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

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

LGTM. @pganssle go for it?

@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.

@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 🙂

@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.

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

@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.

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

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

@pganssle call the ball

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!

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