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

Add type to OrderedDict #1954

Merged
merged 1 commit into from Jun 2, 2021
Merged

Conversation

nakato
Copy link
Contributor

@nakato nakato commented May 17, 2021

OrderedDict needs to be typed just like Dict needs to be typed. The
OrderedDict "input" variable key and value are used to append to a
string without any processing, so it seems unlikely that something other
than a string would be valid.

OrderedDict needs to be typed just like Dict needs to be typed.  The
OrderedDict "input" variable key and value are used to append to a
string without any processing, so it seems unlikely that something other
than a string would be valid.
@codecov-commenter
Copy link

Codecov Report

Merging #1954 (fa28310) into master (d1644e3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1954   +/-   ##
=======================================
  Coverage   98.85%   98.85%           
=======================================
  Files         108      108           
  Lines       11064    11064           
=======================================
  Hits        10937    10937           
  Misses        127      127           

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 d1644e3...fa28310. Read the comment docs.

@s-t-e-v-e-n-k
Copy link
Collaborator

I'm not certain here, but I think input's value can also be bytes. key should always be string. I hope.

@nakato
Copy link
Contributor Author

nakato commented May 17, 2021

I'm not certain here, but I think input's value can also be bytes. key should always be string. I hope.

Wouldn't that be a TypeError?

>>> a = "str"
>>> a += b"bytes"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can only concatenate str (not "bytes") to str
>>> 

@s-t-e-v-e-n-k
Copy link
Collaborator

Not that input's values can be string and bytes -- but I was right, you need to check more closely, a quick grep showed me at least one call site where the value is also a boolean.

@nakato
Copy link
Contributor Author

nakato commented May 17, 2021

a quick grep showed me at least one call site where the value is also a boolean.

How does that work? Python throws a TypeError Exception for a += True too.

__request_encode does not do any processing to input, and that calls the encode function defined inside requestMultipart, it doesn't do any pre-processing on input either.

            encoded_input = ""
            for name, value in input.items():
                encoded_input += f"--{boundary}{eol}"
                encoded_input += f'Content-Disposition: form-data; name="{name}"{eol}'
                encoded_input += eol
                encoded_input += value + eol
            encoded_input += f"--{boundary}--{eol}"
            return f"multipart/form-data; boundary={boundary}", encoded_input

@nakato
Copy link
Contributor Author

nakato commented May 17, 2021

but I was right, you need to check more closely, a quick grep showed me at least one call site where the value is also a boolean.

Where? I don't see that use.

% grep -Ri requestMultipart github tests                                                     
github/Repository.py:        headers, output = self._requester.requestMultipartAndCheck(
github/Requester.py:    def requestMultipartAndCheck(
github/Requester.py:            *self.requestMultipart(
github/Requester.py:    def requestMultipart(
github/Requester.pyi:    def requestMultipart(
github/Requester.pyi:    def requestMultipartAndCheck(

The only use of it I see, including grepping the tests, is in Repository.py, and that is gated with (runtime) asserts that ensure everything used to create post_parameters are str.

@s-t-e-v-e-n-k
Copy link
Collaborator

These request methods all look the damned same, I swear ....

@s-t-e-v-e-n-k s-t-e-v-e-n-k merged commit ed7d0fe into PyGithub:master Jun 2, 2021
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.

None yet

3 participants