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

Convert to Python 3 #1128

Merged
merged 57 commits into from
Nov 15, 2020
Merged

Convert to Python 3 #1128

merged 57 commits into from
Nov 15, 2020

Conversation

jleveque
Copy link
Contributor

@jleveque jleveque commented Sep 22, 2020

- What I did

Conform syntax to support Python 3 using 2to3 tool and manual assessment

- How I did it

Using 2to3 tool as well as manual assessment

- How to verify it

Build a Python 3 version of the sonic-utilities package, install and test all applications

NOTE: This cannot currently be done, as sonic-utilities depends on sonic-config-engine and sonic-yang-mgmt, neither of which are we currently building Python 3 versions of.

  • Need Python 3 version of sonic-config-engine
  • Need Python 3 version of sonic-yang-mgmt

@jleveque jleveque self-assigned this Sep 22, 2020
@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

@@ -545,7 +544,7 @@ def _mergeItems(it1, it2):
pass
return

for it in D1.keys():
for it in list(D1.keys()):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for it in list(D1.keys()):
for it in D1:

Copy link
Contributor Author

@jleveque jleveque Oct 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work. It would need to be:

for it, _ in D1:

I will not make a change like this in this PR. Plus, keeping .keys() makes it clear what the intention is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will work. I think you are talking about

for it, _ in D1.items():

In reply to: 499074240 [](ancestors = 499074240)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're correct. I misread this one. Everything started blending together after all these changes :)

I will go back through and do a cleanup pass in the future.

Copy link
Contributor Author

@jleveque jleveque Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have opened a PR to clean up here.

@@ -577,7 +576,7 @@ def _searchKeysInConfig(self, In, Out, skeys):
'''
found = False
if isinstance(In, dict):
for key in In.keys():
for key in list(In.keys()):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for key in list(In.keys()):
for it in In:

Copy link
Contributor Author

@jleveque jleveque Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work. It would need to be:

for it, _ in In:

I will not make a change like this in this PR. Plus, keeping .keys() makes it clear what the intention is.

clear/main.py Outdated Show resolved Hide resolved
config/kube.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
@@ -91,7 +91,7 @@ def _get_breakout_options(ctx, args, incomplete):

def shutdown_interfaces(ctx, del_intf_dict):
""" shut down all the interfaces before deletion """
for intf in del_intf_dict.keys():
for intf in list(del_intf_dict.keys()):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for intf in list(del_intf_dict.keys()):
for intf in del_intf_dict:

Copy link
Contributor Author

@jleveque jleveque Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work. It would need to be:

for intf, _ in del_intf_dict:

I will not make a change like this in this PR. Plus, keeping .keys() makes it clear what the intention is.

config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
@@ -645,7 +645,7 @@ def _get_disabled_services_list(config_db):

feature_table = config_db.get_table('FEATURE')
if feature_table is not None:
for feature_name in feature_table.keys():
for feature_name in list(feature_table.keys()):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list [](start = 28, length = 4)

The same

Copy link
Contributor Author

@jleveque jleveque Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work. It would need to be:

for feature_name, _ in feature_table:

I will not make a change like this in this PR. Plus, keeping .keys() makes it clear what the intention is.

config/main.py Outdated
@@ -753,23 +753,23 @@ def _restart_services(config_db):

def interface_is_in_vlan(vlan_member_table, interface_name):
""" Check if an interface is in a vlan """
for _,intf in vlan_member_table.keys():
for _,intf in list(vlan_member_table.keys()):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list [](start = 18, length = 4)

The same

Copy link
Contributor Author

@jleveque jleveque Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work. It would need to be something like:

for (_,intf), _ in vlan_member_table:

I will not make a change like this in this PR. Plus, keeping .keys() makes it clear what the intention is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I did add a space after the comma.

config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

jleveque added a commit to Azure/sonic-build-tools that referenced this pull request Nov 14, 2020
Only test and build Python 3 package of sonic-utilities. Eliminate Python 2 build and do not install Python 2 dependencies.

sonic-net/sonic-utilities#1128 is silently passing all Python 3 unit tests (see output here). Once this PR is merged, then the check build will pass, allowing that PR to merge.
@lgtm-com
Copy link

lgtm-com bot commented Nov 14, 2020

This pull request fixes 8 alerts when merging 0dba073cabd8479f92a446f095c7355795f29841 into 8079558 - view on LGTM.com

fixed alerts:

  • 6 for Variable defined multiple times
  • 2 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Nov 14, 2020

This pull request fixes 8 alerts when merging 800350f into 8079558 - view on LGTM.com

fixed alerts:

  • 6 for Variable defined multiple times
  • 2 for Unused local variable

@jleveque jleveque marked this pull request as ready for review November 15, 2020 00:39
@jleveque jleveque changed the title Support Python 3 syntax Convert to Python 3 Nov 15, 2020
Copy link
Contributor

@lguohan lguohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants