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

[201911] DellEMC platform API 2.0 for Z9264f, S5232f #5637

Merged
merged 7 commits into from
Oct 28, 2020

Conversation

aravindmani-1
Copy link
Contributor

- Why I did it
Added API 2.0 in 201911 branch
- How to verify it
Executed necessary API's through python shell
s5232_API_2.0_UT.txt
z9264f_API_2.0_UT.txt

  • 201811
  • 201911
  • 202006

@lgtm-com
Copy link

lgtm-com bot commented Oct 15, 2020

This pull request introduces 36 alerts when merging a173745 into 44abb50 - view on LGTM.com

new alerts:

  • 10 for Wrong number of arguments in a class instantiation
  • 8 for Unused import
  • 6 for Except block handles 'BaseException'
  • 2 for Unused local variable
  • 2 for Unreachable code
  • 2 for Variable defined multiple times
  • 2 for Wrong name for an argument in a class instantiation
  • 1 for Module imports itself
  • 1 for Syntax error
  • 1 for 'import *' may pollute namespace
  • 1 for Result of integer division may be truncated

@lgtm-com
Copy link

lgtm-com bot commented Oct 15, 2020

This pull request introduces 26 alerts when merging 459f49f into 44abb50 - view on LGTM.com

new alerts:

  • 10 for Wrong number of arguments in a class instantiation
  • 5 for Unused import
  • 2 for Unused local variable
  • 2 for Unreachable code
  • 2 for Variable defined multiple times
  • 2 for Wrong name for an argument in a class instantiation
  • 1 for Module imports itself
  • 1 for 'import *' may pollute namespace
  • 1 for Result of integer division may be truncated

@lgtm-com
Copy link

lgtm-com bot commented Oct 15, 2020

This pull request introduces 15 alerts when merging 53bbc02 into 44abb50 - view on LGTM.com

new alerts:

  • 10 for Wrong number of arguments in a class instantiation
  • 2 for Wrong name for an argument in a class instantiation
  • 1 for Module imports itself
  • 1 for 'import *' may pollute namespace
  • 1 for Result of integer division may be truncated

@lgtm-com
Copy link

lgtm-com bot commented Oct 15, 2020

This pull request introduces 14 alerts when merging d55dc7c into 44abb50 - view on LGTM.com

new alerts:

  • 10 for Wrong number of arguments in a class instantiation
  • 2 for Wrong name for an argument in a class instantiation
  • 1 for Module imports itself
  • 1 for 'import *' may pollute namespace

@lguohan
Copy link
Collaborator

lguohan commented Oct 15, 2020

can you fix the ltgm alerts?

@aravindmani-1
Copy link
Contributor Author

Hi @lguohan ,
The first alarm seems to be false warning. I'm passing correct number of argument only.

10 for Wrong number of arguments in a class instantiation.

  • When clicking the values field in lgtm link, it redirects to sfp.py defined for Dell S6000 platform.

  •      root/platform/broadcom/sonic-platform-modules-dell/s6000/sonic_platform/sfp.py
    
  • When clicking the psu,thermal,component__init__ link redirects to API 2.0 files defined in Mlnx platform

  •     root/platform/mellanox/mlnx-platform-api/sonic_platform/(psu/thermal/component).py.
    

@jleveque
Copy link
Contributor

jleveque commented Oct 15, 2020

Yes, this seems to be a bug/limitation of LGTM. With the platform API, we have multiple definitions of each class, provided by each vendor. LGTM generates "Wrong number of arguments in a class instantiation" alerts because when it attempts to find the class definition, it finds a different vendor's implementation, and if they modified the definition, it thinks there is a problem and generates a false alarm. There's not much we can do about this unless LGTM makes their algorithm smarter.

@lgtm-com
Copy link

lgtm-com bot commented Oct 16, 2020

This pull request introduces 16 alerts when merging 40f1ba7 into 36ea042 - view on LGTM.com

new alerts:

  • 10 for Wrong number of arguments in a class instantiation
  • 2 for Module imports itself
  • 2 for 'import *' may pollute namespace
  • 2 for Wrong name for an argument in a class instantiation

@aravindmani-1
Copy link
Contributor Author

@jleveque , @lguohan fixed the applicable LGTM alerts.
Please let me know if any modification is required.

@lgtm-com
Copy link

lgtm-com bot commented Oct 16, 2020

This pull request introduces 16 alerts when merging 87f73a9 into 36ea042 - view on LGTM.com

new alerts:

  • 10 for Wrong number of arguments in a class instantiation
  • 2 for Module imports itself
  • 2 for 'import *' may pollute namespace
  • 2 for Wrong name for an argument in a class instantiation

@aravindmani-1
Copy link
Contributor Author

retest broadcom please

1 similar comment
@aravindmani-1
Copy link
Contributor Author

retest broadcom please

@aravindmani-1
Copy link
Contributor Author

retest broadcom please

1 similar comment
@aravindmani-1
Copy link
Contributor Author

retest broadcom please

@lgtm-com
Copy link

lgtm-com bot commented Oct 22, 2020

This pull request introduces 16 alerts when merging 2cfd812 into ea28f2d - view on LGTM.com

new alerts:

  • 10 for Wrong number of arguments in a class instantiation
  • 2 for Module imports itself
  • 2 for 'import *' may pollute namespace
  • 2 for Wrong name for an argument in a class instantiation

@aravindmani-1
Copy link
Contributor Author

retest mellanox please

@aravindmani-1
Copy link
Contributor Author

retest broadcom please

@aravindmani-1
Copy link
Contributor Author

retest mellanox please

@jleveque jleveque changed the title DellEMC platform API 2.0 for Z9264f,S5232f [201911] DellEMC platform API 2.0 for Z9264f, S5232f Oct 28, 2020
@jleveque jleveque merged commit 3734bf3 into sonic-net:201911 Oct 28, 2020
@aravindmani-1 aravindmani-1 deleted the port_2.0_changes branch October 29, 2020 15:18
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