Skip to content

CI: Revive Banklist Tests #42889

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 9 commits into from
Sep 1, 2021
Merged

CI: Revive Banklist Tests #42889

merged 9 commits into from
Sep 1, 2021

Conversation

lithomas1
Copy link
Member

@lithomas1 lithomas1 commented Aug 4, 2021 •
edited by alimcmaster1
Loading

@pep8speaks
Copy link

pep8speaks commented Aug 4, 2021 •
edited
Loading

Hello @lithomas1! 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-09-01 04:50:03 UTC

@lithomas1 lithomas1 marked this pull request as draft August 4, 2021 21:33
@jreback jreback added IO HTML read_html, to_html, Styler.apply, Styler.applymap Testing pandas testing functions or related to the test suite labels Aug 5, 2021
alimcmaster1
alimcmaster1 requested changes Aug 25, 2021
View reviewed changes
Copy link
Member

@alimcmaster1 alimcmaster1 left a comment

Choose a reason for hiding this comment

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

Can you also fix up PEP8 and see if the test failures are fixable?

@lithomas1 - happy to help review this one

@alimcmaster1 alimcmaster1 self-assigned this Aug 25, 2021
@jreback
Copy link
Contributor

jreback commented Sep 1, 2021

can you rebase

@lithomas1
Copy link
Member Author

This is un-mergeable in its current state because the lxml engine is inconsistent with the other engines which do read this in properly. It has to do with the attrs keyword, which when unspecified makes lxml return the correct result.

I do not have much time to patch the lxml engine right now because school has started again, and I am mostly focusing my efforts on other PRs.

Because the doc update just does a plain read_html, I can update the PR to do a doc update only, if you want.

@jreback
Copy link
Contributor

jreback commented Sep 1, 2021

i see
we can just remove those 2
tests entirely as well

@lithomas1
Copy link
Member Author

@github-actions pre-commit.

@lithomas1 lithomas1 marked this pull request as ready for review September 1, 2021 13:38
@lithomas1
Copy link
Member Author

I removed the attrs checking component, and commented it. An incomplete test is better than no test I guess...

@lithomas1 lithomas1 added this to the 1.4 milestone Sep 1, 2021
@jreback
Copy link
Contributor

jreback commented Sep 1, 2021

thanks looks fine. @alimcmaster1 if any comments.

@alimcmaster1 alimcmaster1 changed the title CI/DOC Revive Banklist CI: Revive Banklist Tests Sep 1, 2021
@alimcmaster1 alimcmaster1 merged commit 9981172 into pandas-dev:master Sep 1, 2021
@alimcmaster1
Copy link
Member

thanks @lithomas1 - LGTM

attack68 pushed a commit to attack68/pandas that referenced this pull request Sep 1, 2021
@lithomas1 @attack68
CI: Revive Banklist Tests (pandas-dev#42889)
fe30ac7 
(cherry picked from commit 9981172)
feefladder pushed a commit to feefladder/pandas that referenced this pull request Sep 7, 2021
@lithomas1 @feefladder
CI: Revive Banklist Tests (pandas-dev#42889)
f781907
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alimcmaster1 alimcmaster1 Awaiting requested review from alimcmaster1

@jreback jreback Awaiting requested review from jreback

Assignees

@alimcmaster1 alimcmaster1

Labels
IO HTML read_html, to_html, Styler.apply, Styler.applymap Testing pandas testing functions or related to the test suite
Projects
None yet
Milestone
1.4
Development

Successfully merging this pull request may close these issues.

BUG: Banklist.html was removed, failure on master
4 participants
@lithomas1 @pep8speaks @jreback @alimcmaster1