Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
added static typing to data_utils.py #662
added static typing to data_utils.py #662
Changes from 18 commits
84b8fc4
757076b
8a652e7
23bb6ac
bc44f0d
cc1b2d0
ff82975
9bd01a4
46a6712
99ea3e1
48c192a
e4d46f5
30381bb
d12d987
efe5fde
cdadc48
8f390c3
796dd83
1654351
5ca9305
7d5b8fb
7fe1704
e0cf544
849263b
9485c9d
bb9807f
8de0cef
351462b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
did we try overloading
unicode_to_str
? would that fix this so we don't have to cast?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.
I did some experimentation and added this:
The issue I'm getting now is when we call this
I get
from mypy. My guess is that we can solve this by casting before we pass in the function to
object_hook
but that would somewhat defeat the purpose of overloading in the first place.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.
@Sanketh7 I think you don't need to specify:
b/c of the three other overloads. I assume the defaults would still need to be assigned though
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.
Even with that removal, I'm still getting the same issue. I think it has to do with the function taking on 3 possible types while
object_hook
is only compatible with one of those types.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.
This issue no longer applies due to the new way of handling json types.
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.
Is this comment needed? isnt that what the type cast is doing?
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.
that's fair ... not really needed per se. I'm fine leaving just for clarity in the code
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.
I added it because
cast
is inherently unsafe but it turns outobj
will always be aDict
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.
Is this bc of the
OrderedDict
?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.
What's our guarantee that it is a dict?
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.
I've determined this could be a list. however, not a string.
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.
The code that could result in this would be:
Currently, there's an error, in the reader not supporting this. However, it should.
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.
should be resolved in PR #691
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.
No longer a need for casting from my latest commit.
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.
similar to below, if we override allowing each diff input, does that remove the need to cast?
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.
I don't think so because as far I can tell, overloading doesn't let us have multiple implementations. It just lets us specify relationships between input types and return types.
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.
in certain conditions, would still need to cast I think because the code doesn't know that under this condition the type should be this. So that's where
cast()
comes in to play under specificif
conditions... at least that's how I'm reading it.LGTM
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.
does it make more sense to cast to string immediately? On line 653 could we do
search_query_value = case(str, b"\n")
then we don't need line 660 and search_query_value can just be statically typed asstr
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.
I see what you are getting at... wouldn't say its a high priority. We / I can fix in a follow-up PR though
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.
Instead of casting, could nth loc just accept bytes?
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.
@Sanketh7
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.
We could make
find_nth_loc
accept bytes but then we'd have to deal with the cases where the search string is of typestr
and the search query is of typebytes
(and vice versa). One way I found to deal with this is to use the typeAnyStr
infind_nth_loc
which basically creates a generic type that can only bestr
orbytes
. However, the issue I run into is that the current way the code is set up, we would have to pass in aUnion[str, bytes]
tofind_nth_loc
but you can't pass that in forAnyStr
(see python/mypy#1533).