Conversation
|
This pull request introduces 1 alert when merging e7e6598 into 2e024de - view on LGTM.com new alerts:
|
|
please add unit tests and also fix lgtm alerts. |
lguohan
left a comment
There was a problem hiding this comment.
please add unit tests and also fix lgtm alerts.
geans-pin
left a comment
There was a problem hiding this comment.
Code looks ok, will approve once UT log is provided
|
|
||
| def display_eeprom(self): | ||
| click.echo(self.output) | ||
| @multi_asic_util.run_on_multi_asic |
There was a problem hiding this comment.
Add blank line above this
| def cli(): | ||
| """sfpshow - Command line utility for display SFP transceivers information""" | ||
| pass | ||
| # 'summary' subcommand |
There was a problem hiding this comment.
Add blank line above this
Added spaces
This comment has been minimized.
This comment has been minimized.
|
Please update the command reference to include the new CLI. show interfaces transceiver summary |
| sub = \ | ||
| [interface, sfp_info_dict.get('display_name', 'N/A'), sfp_info_dict.get('power_rating_max', 'N/A'), | ||
| sfp_info_dict.get('vendor_name', 'N/A'), sfp_info_dict.get('vendor_serial_number', 'N/A'), | ||
| sfp_info_dict.get('vendor_part_number', 'N/A'), sfp_info_dict.get('qsa_adapter', 'N/A'), sfp_info_dict.get('qualified', 'N/A')] |
There was a problem hiding this comment.
Is it possible to make the "Qualified" field in the show command optional, via a platform.json parameter? Concerned that it may cause confusion among users and vendors if vendors don’t implement it.
- What I did
Added CLI which summarizes the info for media based on new parser
Parser PR is sonic-net/sonic-platform-common#129
- How I did it
Extended CLICK
- How to verify it
Visually verified it against expected values
- Previous command output (if the output of a command-line utility has changed)
Nil
- New command output (if the output of a command-line utility has changed)