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

Typing: Narrow down types of arguments (NDFrame) #10 #52754

Merged
merged 16 commits into from
Apr 25, 2023
Merged

Typing: Narrow down types of arguments (NDFrame) #10 #52754

merged 16 commits into from
Apr 25, 2023

Conversation

mKlepsch
Copy link
Contributor

@mKlepsch mKlepsch commented Apr 18, 2023

Typing: Narrow down types of arguments (NDFrame) #10
Made these specifications to Literals:

  • [ X] Specify date_unit in to_json more precisely
  • [ X] Specify mode in to_hdf more precisely
  • [ X] Specify complib in to_hdf more precisely
  • [ X] Specify format in to_hdf more precisely

@phofl @jorisvandenbossche @noatamir @MarcoGorelli

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks for your pr

@@ -2276,7 +2276,7 @@ def to_json(
date_format: str | None = None,
double_precision: int = 10,
force_ascii: bool_t = True,
date_unit: str = "ms",
date_unit: Literal["s", "ms", "us", "ns"] = "ms",
Copy link
Member

Choose a reason for hiding this comment

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

this one seems broadly useful - perhaps let's define it as TimeUnit in _typing.py and import it here?

complevel: int | None = None,
complib: str | None = None,
complib: ["zlib", "lzo", "bzip2", "blosc"] | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

missing literal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change addressed problems

@noatamir noatamir added the Sprints Sprint Pull Requests label Apr 18, 2023
@noatamir
Copy link
Member

Thanks for the PR @mKlepsch! The CI caught some errors which I think you can solve locally if you run pre-commit. Let us know if you need help.

@mKlepsch
Copy link
Contributor Author

Hope it works now!

@mKlepsch
Copy link
Contributor Author

Hi, I can't figure out why it is still failing the CI. Could you give me some pointers?

@MarcoGorelli
Copy link
Member

you'll need to run the pre-commit hooks, please check the contributing guide for how to do that

@phofl
Copy link
Member

phofl commented Apr 22, 2023

You can do:

pre-commit install
pre-commit run --all-files

(It's probably a space at the end of the line, which is disallowed

Copy link
Member

@phofl phofl left a comment

Choose a reason for hiding this comment

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

Looks good after fixing pre-commit

@mroeschke mroeschke added this to the 2.1 milestone Apr 24, 2023
@mroeschke mroeschke merged commit aa1f96b into pandas-dev:main Apr 25, 2023
@mroeschke
Copy link
Member

Thanks @mKlepsch

Rylie-W pushed a commit to Rylie-W/pandas that referenced this pull request May 19, 2023
…s-dev#52754)

* improved specificationfor date_unit in to_json more precisely

* improved specification mode in to_hdf

* added the Literal to complib of to_hdf

* improved format for to_hdf

* address PR review

* Update generic.py

removed the new line in the import

* ran "pre-commit" to fix trailing whitespace
Daquisu pushed a commit to Daquisu/pandas that referenced this pull request Jul 8, 2023
…s-dev#52754)

* improved specificationfor date_unit in to_json more precisely

* improved specification mode in to_hdf

* added the Literal to complib of to_hdf

* improved format for to_hdf

* address PR review

* Update generic.py

removed the new line in the import

* ran "pre-commit" to fix trailing whitespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sprints Sprint Pull Requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants