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

Return an informative error message when spec should be a list #62

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hinnefe2
Copy link
Contributor

glom is great, thanks for making it!

This PR makes the error message more informative when your target is a list and your spec is not.

Currently when target is a list but spec is not you get

ValueError: invalid literal for int() with base 10:

which doesn't immediately suggest that you forgot to make your spec a list. I've run into this a couple times now, and each time I forgot about last time and ended up having to check the source code to figure out what was going on. Next time I might finally remember, but for other people hitting this a different error message might help. With this change, the error message is

ValueError: Error indexing the target. This can happen when your target is a list and your spec is not. Did you forget to put brackets around your spec?

Please let me know if this check makes more sense somewhere else, I just put it right where the error first gets raised.

Thanks!

MWE:

from glom import glom

target = [{'xxx': i} for i in range(3)]

spec = {'number': 'xxx'}

glom(target, spec)

When target is a list but spec is not you get

>> ValueError: invalid literal for int() with base 10:

This commit makes the error message more informative in this case.
@codecov-io
Copy link

codecov-io commented Nov 26, 2018

Codecov Report

Merging #62 into master will decrease coverage by 0.11%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
- Coverage   94.45%   94.34%   -0.12%     
==========================================
  Files          14       14              
  Lines        1479     1485       +6     
  Branches      231      232       +1     
==========================================
+ Hits         1397     1401       +4     
- Misses         47       48       +1     
- Partials       35       36       +1
Impacted Files Coverage Δ
glom/core.py 96.48% <71.42%> (-0.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab822e3...d360a9c. Read the comment docs.

@kurtbrose
Copy link
Collaborator

hey this looks great, thanks!

@mahmoud
Copy link
Owner

mahmoud commented Dec 26, 2018

Hey, thanks! I agree with Kurt, this is indeed great. I've been iterating on the idea of having a top-level exception raised to simplify the stack and make the error more actionable. I think this definitely falls into that vein. I'll need to think a bit more about this exception message-based approach. Might this make the behavior a bit version-specific?

@hinnefe2
Copy link
Contributor Author

Hey - sorry for the delayed response! I see what you mean about relying on the exception message, I'm not sure how stable those messages are across python versions.

What about an explicit check on whether int(index) raises a ValueError? That catches the problem I was running into and seems narrow enough not to mask anything else.

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.

4 participants