Skip to content

dateutil first version #133

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

Merged
merged 4 commits into from
Apr 22, 2016
Merged

dateutil first version #133

merged 4 commits into from
Apr 22, 2016

Conversation

jukebox
Copy link
Contributor

@jukebox jukebox commented Mar 31, 2016

No description provided.

PERTAIN = ... # type: List[str]
TZOFFSET = ... # type: Dict[str, int]

def __init__(self, dayfirst: bool=..., yearfirst: bool=...) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Style suggestion: in stubs we prefer compactness and it's fine to put the "..." of the method body on the same line and omit the blank lines between methods.

@gvanrossum
Copy link
Member

Hey, sorry for the slow response. I just realized I've never used dateutil so I don't really have any idea whether this stub is adequate or not.

Also, because it's third party, could you ask the dateutil author whether they are okay with having a dateutil stub in typeshed? See the note in README.md under "third_party".

@pganssle
Copy link
Member

The original author of dateutil does not currently maintain the library, but I'm one of the two principal maintainers (and since the other guy has a newborn, probably the one most likely to respond to you) and I'm fine with a stub being included.


def parse(
self,
timestr: str,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not overly familiar with how type annotations work, but you may want to be aware that timestr duck-types the input. In Python 3.x will take bytes, str or stream objects that implement read(). The preferred input is probably str, though.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a union of bytes, str and IO would work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@gvanrossum
Copy link
Member

@jukebox: I don't understand what happened, but it looks like a whole bunch of merged commits ended up in your PR. Could you clean that up (maybe try a rebase) or start a new PR? The comment from #133 (comment) indicates we can go ahead with this PR, but it's currently impossible to review due to the merge noise.

@jukebox
Copy link
Contributor Author

jukebox commented Apr 22, 2016

is it ok now ?

@gvanrossum gvanrossum merged commit f5f349c into python:master Apr 22, 2016
@gvanrossum
Copy link
Member

Yup, thanks! Does dateutil support Python 2 also?

@pganssle
Copy link
Member

pganssle commented Apr 22, 2016

dateutil supports python 2.6+ and 3.2+.

@pganssle
Copy link
Member

@jukebox Are you going to continue filling out the rest of the stub? If not, I am probably in a pretty good position to flesh out most of the library. I'll just need a bit of time to familiarize myself with the exact notation and whatnot.

It seems that these stubs also cover the private interface, which makes sense, but makes things a bit more complicated, I would think, especially given #153.

@jukebox
Copy link
Contributor Author

jukebox commented Apr 27, 2016

@pganssle i think i dont have time to continue the stub and i dont use other methods and old python versions.

@pganssle
Copy link
Member

OK, I'll make a separate PR covering 2.x and expanding coverage to the whole library.

@jukebox jukebox deleted the dateutil branch May 19, 2016 07:56
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