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

Remove use of six #1340

Merged
merged 1 commit into from
Apr 9, 2021
Merged

Remove use of six #1340

merged 1 commit into from
Apr 9, 2021

Conversation

avelichka
Copy link
Contributor

Removes the use of six so it is no longer a dependency and is not imported and used in any modules, tests, or scripts.

Fixes #1296

Description of the changes being introduced by the pull request:

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@jku
Copy link
Member

jku commented Apr 8, 2021

Just two comments (unfortunately one touches a lot of lines):

  • I believe import urllib does not bring urllib.request or urllib.parse into scope -- I think some dependency (requests?) maybe happens to import those so we don't notice this? If I'm correct, I'd be fine with import urllib.request style in tests so you don't need to modify all calls again but in the actual code some form of from urllib import x would probably be preferred for vendoring reasons
  • for key, value in dict.items(mydict): is not wrong... but wouldn't for key, value in mydict.items(): be the normal way to do this? I hope there is a nice regex to do that replace if you end up doing it.

@avelichka
Copy link
Contributor Author

Thank you for the review, @jku! Hopefully, I've managed to address your comments in the two new commits.

@jku
Copy link
Member

jku commented Apr 9, 2021

Thanks, LGTM!

If you want to squash these into one commit and force-push feel free to do so, otherwise I'll do a "squash and merge" (either way is fine by me but in the latter case I can only mark you as co-author as the new commit will then be mine).

Remove use of six

Signed-off-by: Velichka Atanasova <avelichka@vmware.com>

Replace the use of dict.items(mydict) with mydict.items(), dict.keys(mydict) with mydict.keys() and dict.values(mydict) with mydict.values()

Signed-off-by: Velichka Atanasova <avelichka@vmware.com>

Replace 'import urllib' and 'import urllib.x' with 'from urllib import x' for vendor compatibility

Signed-off-by: Velichka Atanasova <avelichka@vmware.com>
@avelichka
Copy link
Contributor Author

I've squashed them into one commit. Thank you, @jku.

@jku jku merged commit 08f48d5 into theupdateframework:develop Apr 9, 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.

Remove use of six
2 participants