Skip to content

TST: fix read_stata doctest #42670 #42701

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 33 commits into from
Jul 31, 2021
Merged

TST: fix read_stata doctest #42670 #42701

merged 33 commits into from
Jul 31, 2021

Conversation

KrishnaSai2020
Copy link
Contributor

@KrishnaSai2020 KrishnaSai2020 commented Jul 24, 2021 •
edited
Loading

fangchenli
fangchenli requested changes Jul 24, 2021
View reviewed changes
Copy link
Member

@fangchenli fangchenli left a comment

Choose a reason for hiding this comment

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

We want them fixed rather than skipped.

@fangchenli fangchenli changed the title doctest fix for #42670 DOC: doctest fix for #42670 Jul 24, 2021
@fangchenli fangchenli changed the title DOC: doctest fix for #42670 TST: fix read_stata doctest #42670 Jul 24, 2021
@KrishnaSai2020
Copy link
Contributor Author

KrishnaSai2020 commented Jul 25, 2021 •
edited
Loading

Yes but the only solution I can see is creating a dummy .dta file for it to work since it needs a file to be read and I don't know where you would store that. For example, should I create an examples folder to store it so all future example documentation code can run as well?

@pep8speaks
Copy link

pep8speaks commented Jul 25, 2021 •
edited
Loading

Hello @KrishnaSai2020! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-07-31 07:38:51 UTC

KrishnaSai2020
KrishnaSai2020 commented Jul 25, 2021
View reviewed changes
@KrishnaSai2020
Copy link
Contributor Author

Made the requested changes @fangchenli.

@bashtage
Copy link
Contributor

bashtage commented Jul 26, 2021 •
edited
Loading

This is ruining the docstring's purpose just to pass a test. It is not a good idea. The entire point of reading with chunks to to parse a large data file. IT should have > 10,000 roww to be illustrative.

@bashtage bashtage added Docs IO Stata read_stata, to_stata labels Jul 26, 2021
@KrishnaSai2020
Copy link
Contributor Author

@bashtage I see what you mean. I made the changes to the df's size please review the new changes.

bashtage
bashtage reviewed Jul 27, 2021
View reviewed changes
@KrishnaSai2020
Copy link
Contributor Author

@bashtage thanks for you help. I have now made the changes.

bashtage
bashtage reviewed Jul 27, 2021
View reviewed changes
bashtage
bashtage requested changes Jul 27, 2021
View reviewed changes
Copy link
Contributor

@bashtage bashtage left a comment

Choose a reason for hiding this comment

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

Some formatting issues

@KrishnaSai2020
Copy link
Contributor Author

KrishnaSai2020 commented Jul 27, 2021 •
edited
Loading

@bashtage I think I've fixed the formatting now. Please do let me know if there is anything else

bashtage
bashtage requested changes Jul 27, 2021
View reviewed changes
Copy link
Contributor

@bashtage bashtage left a comment

Choose a reason for hiding this comment

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

Still a bit of formatting.

@KrishnaSai2020
Copy link
Contributor Author

KrishnaSai2020 commented Jul 27, 2021 •
edited
Loading

@bashtage I've added this doctest into the scripts and resolved the conflicts

@KrishnaSai2020
Copy link
Contributor Author

This should now pass all checks.

@KrishnaSai2020
Copy link
Contributor Author

From what I can tell everything works apart from the doctests in Style_render.py which is unrelated to this pr.

@KrishnaSai2020
Copy link
Contributor Author

KrishnaSai2020 commented Jul 29, 2021 •
edited
Loading

@fangchenli Could I get an update as to what is going to happen with this pr, please? Is it going to get merged? or have I done something wrong which means it can't get merged?

bashtage
bashtage requested changes Jul 29, 2021
View reviewed changes
Copy link
Contributor

@bashtage bashtage left a comment

Choose a reason for hiding this comment

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

Need to remove new module that doesn't pass, and small grammar change

@KrishnaSai2020 KrishnaSai2020 mentioned this pull request Jul 30, 2021
Closed
3 tasks
@KrishnaSai2020
Copy link
Contributor Author

@fangchenli please re-review

@KrishnaSai2020
Copy link
Contributor Author

Could I get a second opinion on this PR, please? it's passed all checks and already has one approval

@bashtage
Copy link
Contributor

@KrishnaSai2020 This LGMT. I'll leave it to @jreback to make the final decision. You don't need to merge master in anymore unless there is a conflict.

@bashtage bashtage merged commit c79e7ee into pandas-dev:master Jul 31, 2021
@bashtage
Copy link
Contributor

@KrishnaSai2020 Thanks.

@KrishnaSai2020 KrishnaSai2020 deleted the doctest-examples-fix branch July 31, 2021 18:05
feefladder pushed a commit to feefladder/pandas that referenced this pull request Sep 7, 2021
@KrishnaSai2020 @feefladder
TST: fix read_stata doctest pandas-dev#42670 (pandas-dev#42701)
14180dd 
* doctest fix for pandas-dev#42670
* added read_stata docstring into the doctests

Co-authored-by: KrishnaSai2020 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bashtage bashtage bashtage approved these changes

@fangchenli fangchenli Awaiting requested review from fangchenli

Assignees
No one assigned
Labels
Docs IO Stata read_stata, to_stata
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TST: Fix doctest in read_stata
4 participants
@KrishnaSai2020 @pep8speaks @bashtage @fangchenli