-
Notifications
You must be signed in to change notification settings - Fork 3
Add warning for base64 encode functions #42
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
Conversation
nanjekyejoannah
left a comment
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.
Main points: Use Py3kWarning Exception and don't prepend Pygrate2
Lib/base64.py
Outdated
|
|
||
| _WARAP_FUNC = ["b64encode", "b32encode", "b16encode", "encodebytes"] | ||
| for _name in _WARAP_FUNC: | ||
| if _name in __all__: |
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.
Are there situations in which _name couldn't be in __all__? I think these functions are always available, on all platforms?
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 order to solve this compactivity issue, would using globals() instead of __all__ be a good solution to it?
`
for _name in ["b64encode", "b32encode", "b16encode", "encodebytes"]:
if _name in globals():
globals()[_name] = _warn_encode(globals()[_name], _name)
`
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.
@Eddy114514 Still wondering about this one.
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.
@ltratt After checking with documentations, I think "b64/b32/b16encode" exists in both 2.6 and 3.x, but I should remove encodebytes as it doesn't exist in python 2.
After removing it, I think it is suitable to remove the if-statement
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.
@ltratt Hi, I am wondering if my new implementation resolve the concern. thanks
|
@Eddy114514 If you could ping me when you've made updates, it'll help me review them quicker (otherwise I won't notice). Reviewing now. |
@ltratt Understood, thanks for pointing it out, I will make sure to ping you with updates going forward, sorry about it. |
Eddy114514
left a comment
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.
add more test cases to cover b32,16 in test_base64
Lib/base64.py
Outdated
|
|
||
| _WARAP_FUNC = ["b64encode", "b32encode", "b16encode", "encodebytes"] | ||
| for _name in _WARAP_FUNC: | ||
| if _name in __all__: |
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 order to solve this compactivity issue, would using globals() instead of __all__ be a good solution to it?
`
for _name in ["b64encode", "b32encode", "b16encode", "encodebytes"]:
if _name in globals():
globals()[_name] = _warn_encode(globals()[_name], _name)
`
Lib/base64.py
Outdated
|
|
||
| _WARAP_FUNC = ["b64encode", "b32encode", "b16encode", "encodebytes"] | ||
| for _name in _WARAP_FUNC: | ||
| if _name in __all__: |
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.
@ltratt After checking with documentations, I think "b64/b32/b16encode" exists in both 2.6 and 3.x, but I should remove encodebytes as it doesn't exist in python 2.
After removing it, I think it is suitable to remove the if-statement
|
Please squash. [Note: this doesn't mean you have to squash things down to a single commit, though you can if you want! It does mean that I'd like you to squash down to what you consider a sensible number of commits. More details of this process at https://ykjit.github.io/yk/contributing/prs.html] |
d77fa98 to
67dcccf
Compare
@ltratt Thank you for your instruction, I have squash my commits |
|
Hmm, I'm not quite sure why that C code isn't building anymore. It's not directly related to this PR, but it will block it being merged. @Eddy114514 are you able to replicate this issue locally? |
@ltratt I am unable to replicate this issue locally, I can successfully build and run pygrate 2 (and test) locally by following steps (make distclean, ./configure, make -j). However it do will throw some warning and note during "make -j". As following: |
|
This might be because we're using a new Debian in CI. Will confirm in the morning with luck! |
|
OK, that failure is a genuine error with this PR I think. @Eddy114514 hopefully you can see the builbot log above. |
@ltratt thanks for checking, after checking the log I figured out the problem. My test cases in test_base64 assume python will run with -3 flag such that it will check warnings. I will remove them and only those in py3kwarn |
56834d3 to
cfae32b
Compare
|
@ltratt I have squashed my change, the failed test file in log is test_base64, I tested again (for it and for all) in local. I think it should be ready to merge. Thank you for your help. |
|
Please squash. |
Add test cases in py3kwarn
cfae32b to
b412e9d
Compare
@ltratt squashed to 1 now. |

Add a wrapper in base64.py that wrap b64encode, b32encode, b16encode, encodebytes, allowing them to warn user the behavior difference between python2 and python3. Methods' name are listed in the warning message.
warning is as following:

Add test functions in test_base64.py to test whether warning messages shows up.