Check for key in dictionary without magic value comparison

Question

I'm using pydantic in a project to check file formatting for an object, as well as pylint for linting. Whenever I read a file with an invalid format, I want to raise an exception with a descriptive message. The code I'm using looks something like this:

from typing import TypedDict
import pydantic
@pydantic.with_config(pydantic.ConfigDict(extra="forbid"))
class AuthFile(TypedDict):
 key1: str
 key2: int
auth_file = AuthFile(key1='abc', key2=123)
ta = pydantic.TypeAdapter(AuthFile)
try:
 ta.validate_python(auth_file)
except pydantic.ValidationError as ve:
 errors = ve.errors(
 include_url=False,
 include_context=False,
 )
 messages = []
 for error in errors:
 message = f"{error['type']} at {'.'.join([str(loc) for loc in error['loc']])}."
 if "input" in error:
 message = f"{message} Got {error['input']}"
 messages.append(message)
 message = "\n".join(messages)
 error_locs = {
 error["loc"][0] if len(error["loc"]) > 1 else "" for error in errors
 }

And it works just fine. My problem is that

if "input" in error

is a magic value comparison. I understand why, and I know that I could do something like

VE_ERROR_INPUT_KEY = "input"
for error in errors:
 message = f"{error['type']} at {'.'.join([str(loc) for loc in error['loc']])}."
 if VE_ERROR_INPUT_KEY in error:
 message = f"{message} Got {error['input']}"

to "solve" the problem. However, I think that creating a constant for a dict key of a third-party lib to use in a single comparison isn't good practice.

I don't want to turn magic value comparison off, since most of the time I think its good practice, but I also don't want to create a constant for one comparison. My ideal solution is to change how this check is made, but I couldn't find another way. Should I just turn the linter off for that (and other similar) lines?

• AJNeufeld: I have a hard time believing this is actually the code you are using. {'.'.join(str(error['loc']))} would return 'S.o.m.e. .o.b.j.e.c.t. .s.t.r.i.n.g.i.f.i.c.a.t.i.o.n' which should be blatantly obvious if you've tested any error handling.
• BobVitorBob: @AJNeufeld In this context error['loc'] is a tuple[str] with the path to where a problem happened. '.'.join(str(error['loc'])) returns a representation of where the problem is in the dictionary.
• AJNeufeld: Sure. Say error[loc] is (‘foo’, ‘bar’, ‘baz’). Then str(error[loc]) becomes "(‘foo’, ‘bar’, ‘baz’)" (the string representation of the tuple), and ’.’.join(iterable), where in this case the iterable is the above string of characters, produces the each character of the string with the period between each one! So "(.‘.f.o.o.’.,. .‘.b.a.r.’.,. .‘.b.a.z.’.)", which is likely not the desired result.
• BobVitorBob: Oh, you're right, thanks for pointing it out! Edited the post to achieve the right behavior.

Answer 1

I think that creating a constant for a dict key of a third-party lib to use in a single comparison isn't good practice.

I agree.

comment

The standard way to model "I disagree with the linter on this one line" is to add an ignore comment.

 if "input" in error: # pylint: disable=magic-value-comparison

Or use R2004 if you prefer.

EAFP

 try:
 message += f" Got {error['input']}"
 except KeyError:
 pass
 messages.append(message)

non-magic comparison

 e = error.get("input")
 if e:
 message += f" Got {e}"

One could walrus this if desired.
if e := ...
But I wouldn't.

linter

Consider using ruff. It won't get in your way. Very seldom will I disagree with one of its pronouncements. Plus... it's fast!

• BobVitorBob: I actually started using ruff before seeing your answer and I agree its way better. Thanks for the answer!

Answer 2

message creation

This list creation ([...]) is unnecessary in this code:

 message = f"{error['type']} at {'.'.join([str(loc) for loc in error['loc']])}."

A generator expression may be used, eliminating the unnecessary list construction:

 message = f"{error['type']} at {'.'.join(str(loc) for loc in error['loc'])}."

But this can be future simplified to:

 message = f"{error['type']} at {'.'.join(map(str, error['loc']))}."

However, based on the comment by the author:

"In this context error['loc'] is a tuple[str] with the path to where a problem happened."

The calls to str(...) are actually unnecessary, as the elements of the tuple are already strings. Therefore, this can simply become:

 message = f"{error['type']} at {'.'.join(error['loc'])}."

Demo:

>>> error = {'type': 'test', 'loc': ('foo', 'bar', 'baz')}
>>> f"{error['type']} at {'.'.join(error['loc'])}."
'test at foo.bar.baz.'

Off-by-One error?

I am confused about this code:

 error_locs = {
 error["loc"][0] if len(error["loc"]) > 1 else "" for error in errors
 }

Construct a set of error locations using:

• for each error in the errors container:
  • the first element of the error["loc"] tuple, if there are 2 or more elements
  • an empty string otherwise.

Did you really mean len( ) > 1, and not either len( ) >= 1 or len( ) > 0?

If so, you don't even need to get the length of the tuple. Empty tuples are False, non-empty ones are True, so:

 error_locs = {
 error["loc"][0] if error["loc"] else "" for error in errors
 }

• BobVitorBob: Thanks for the answer! I need to cast to str because error['loc'] is typed as tuple[int|str, ...], so '.'.join(error['loc']) complains that it can receive an int. The cast is mostly a way to silence this warning since I know it'll never be an int. Didn't know about the map solution though, that's way nicer. Looking at the answer and reviewing the code, there are some other problems with it that need to be checked. Thanks for your time!