Support fingerprinting of UnsetBool options.
#4665
Conversation
This was a landmine waiting to go off. All other supported option custom types were handled.
|
|
||
|
|
||
| class Encoder(json.JSONEncoder): | ||
| _UNSET_BOOL_ENCODING = '__type::{}::{}'.format(inspect.getmodule(UnsetBool), UnsetBool.__name__) |
There was a problem hiding this comment.
Why is the inspect magic needed? Couldn't this just be UnsetBool.__module__, or even a hard-coded string? I mean, apart from readability concerns _UNSET_BOOL_ENCODING can be literally any unlikely-to-collide string, if I understand this correctly?
There was a problem hiding this comment.
It could be anything - eye of beholder I guess. I look at __module__, __package__ and throw up my hands as to which will be available when. I figure inspect gets this right robustly.
There was a problem hiding this comment.
Well, as long as it's not a performance liability, which it probably isn't. My suspicion of introspection is a JVM holdover, in Python introspection is part of the bargain, I guess.
There was a problem hiding this comment.
Do we actually care what module this is in though? Would we want to invalidate if we renamed the module?
There was a problem hiding this comment.
Well, as long as it's not a performance liability ....
Its a class constant, so no real concern
Do we actually care what module this is in though?
No, just that it doesn't collide with other strings. Open to a concrete suggestion on what value makes sense.
There was a problem hiding this comment.
I think '_UNSET_BOOL_ENCODING' as the value makes sense?
Sorry to harp on this, I just imagine (or another casual reader) looking at this in the future and wondering why we want to invalidate options fingerprints if we change the name or location of an implementation detail. A constant literal string is fairly idiomatic for "symbolic constant value whose actual content isn't significant, only its uniqueness".
There was a problem hiding this comment.
Sounds good - unlikely to collide.
This was a landmine waiting to go off. All other supported option custom
types were handled.