-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
Be specific about more indexes or values #35190
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
Conversation
Oh why not make specific errors instead of relying on value of error message... I'll patch it up at some point |
i've pre-pended draft as I cannot find the button to make this a draft, which I was sure GitHub added to their old UI |
3237221
to
993ca4b
Compare
Hello @Lewiscowles1986! 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 2020-07-14 16:05:47 UTC |
funnily enough, getting the suite to compile and build on my windows laptop has been the largest hurdle. Lots of required C libraries to build and what borders on insane advice (copying executables & libraries to a magic path, rather than putting in path). It's a rather large test suite which generates far too much noise to immediately fail or silence in a helpful way. The makefile and |
fbfc1c8
to
af2f311
Compare
unsure if it needs a whatsnew entry. Technically I did add a new type of exception, but until I get a review (I'd like the suite to pass first), I've no idea. I'm guessing that there is code that relies on this behavior causing some of the environments to fail and not others. I'll try rebasing now master is passing |
Note for reviewer. I am happy of course squash the commits if you this is acceptable and required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you switch back to a ValueError and add a note (improved error message for invalid construction) in whatsnew 1.1 bug fixes in construction can get this in.
Now it's passing, I might like to suggest my initial wording may not have been as helpful as I'd have liked. I actually didn't understand the original message or surrounding code very well.
and
Avoid the formatting, but improve readability from a case-study of 2 people (I could borrow a third). I know naming and explaining is hard, but the cognitive load seems too high from the original, and I think these last suggestions could represent a clearer future desired-state. (For now I'm happy with the PR. It's less test wrangling, and at least stops me dropping to a debugger) |
1c8e22c
to
0d226f9
Compare
I've altered the commit message to be the same as the suggested change-log entry for 1.10 and squashed. |
doc/source/whatsnew/v1.1.0.rst
Outdated
@@ -1169,6 +1169,7 @@ Other | |||
- Bug in :class:`Tick` comparisons raising ``TypeError`` when comparing against timedelta-like objects (:issue:`34088`) | |||
- Bug in :class:`Tick` multiplication raising ``TypeError`` when multiplying by a float (:issue:`34486`) | |||
- Passing a `set` as `names` argument to :func:`pandas.read_csv`, :func:`pandas.read_table`, or :func:`pandas.read_fwf` will raise ``ValueError: Names should be an ordered collection.`` (:issue:`34946`) | |||
- Improved error message for invalid construction (:issue:`35190`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you mention the object that caused the invalid construction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved error message for invalid addition of index? (To be honest that is as clear as mud)
I'll be honest I don't know that I understand what you're asking here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would be better.
It'd just be clearer to know Improved error message for invalid construction
of what?
looks fine ex @mroeschke comment. |
thanks @Lewiscowles1986 |
Thanks all. Comms can be hard, so thank you all for your patience. @jreback @mroeschke 🚀 |
The generic "are not equal" really frustrated me just now when someone was conveying their issue.
This specific two errors lets me know which way around, although I believe having more indexes than data shows a place in the api for a default value (possibly per-index) which would allow this to later be reduced to the case I feel I fully understand of more data than indexes, which I think probably is unsolvable generally.
I can see that both have problems, but I still think it's more important to be specific about which side is mismatched.
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff