Fix type of evaluated_value on string to allow bytes
#721
Conversation
|
Hi @ljodal! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
ljodal
changed the title
evaluated_value on string
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thanks for the PR; this seems reasonable to me but let's fix the type errors this introduces before merging. There are instructions in the readme about how you can set this up on your local machine without relying on CI. |
|
I tried to set everything up locally, but I got some kind of json decode error when installing some of the dev dependencies (left my laptop at the office, so I don't have the error in from of me). I'm on an M1 Mac, so might be something there? Anyway, I can try again after the weekend :) |
|
I'm unable to get Running Compile errorI could try to fix the errors, but not super keen on doing that without type checker support. Let me know if you'd rather I just open an issue? |
|
Let me see if I can get you some help for that Pyre issue. About the maturin build problem: that's interesting, I've never seen that issue before (admittedly, I don't have an arm64 macbook to test on). based on this thread maybe you have an x64 rust toolchain installed as the default on your arm64 machine? You can check by running |
|
Yeah, my entire tool chain is native arm64. Doing |
This can return bytes if the string is a bytestring, e.g.:
In [1]: import libcst as cst
In [2]: cst.parse_expression('b"foo"').evaluated_value
Out[2]: b'foo'
ljodal
force-pushed
the
fix-evaluated-value-type
branch
from
18ec1af
to
7b0b417
Compare
ljodal
force-pushed
the
fix-evaluated-value-type
branch
from
431f6fa
to
eddd5b8
Compare
|
@zsol I took a second stab at this now and got pyre 0.9.10 running locally, so I've fixed the errors I got with that (hopefully it should be all) |
| return left_val + right_val | ||
| if isinstance(left_val, str) and isinstance(right_val, str): | ||
| return left_val + right_val | ||
| return None |
There was a problem hiding this comment.
Not entirely sure what to do with mixed values here, but I assume that's invalid Python and that returning None should be fine?
|
@zsol Any chance of a review of this? Would be nice to get this into one of the upcoming releases :) |
| @@ -1700,6 +1700,8 @@ def get_docstring_impl( | |||
| evaluated_value = val.evaluated_value | |||
| else: | |||
| return None | |||
| if isinstance(evaluated_value, bytes): | |||
| return None | |||
There was a problem hiding this comment.
I had to look up the specs for docstrings. Looks like they are guaranteed to be string literals (as opposed to bytes literals), so this is fine (although I personally would've written if not isinstance(evaluated_value, str))!
zsol
changed the title
evaluated_value on stringevaluated_value on string to allow bytes
Summary
This can return bytes if the string is a bytestring, e.g.: