p4lang / p4runtime-shell

An interactive Python shell for P4Runtime
Apache License 2.0
76 stars 40 forks source link

Add option to make output quieter #115

Closed jafingerhut closed 1 year ago

jafingerhut commented 1 year ago

This is only a draft of a change to show the kind of change I am thinking about -- I do not expect this PR to be in its final form for merging.

The print() calls spread throughout shell.py, while probably exactly what someone wants when using p4runtime-shell interactively, can be fairly noisy in a situation where one is using p4runtime-shell in a batch situation, e.g. a PTF test. In such a context, it would be nice to have an option to make p4runtime-shell not do all of these print calls.

If this seems reasonable, I am open to suggestions on exactly how to control this printing on/off. For example, it would probably be nice for backwards compatibility to have these print calls enabled by default, and require some extra configuration call to the library that disables them.

codecov-commenter commented 1 year ago

Codecov Report

Merging #115 (b9bf2d0) into main (2603e13) will increase coverage by 0.04%. The diff coverage is 73.33%.

:exclamation: Current head b9bf2d0 differs from pull request most recent head f4916d9. Consider uploading reports for the commit f4916d9 to get more accurate results

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #115      +/-   ##
==========================================
+ Coverage   68.30%   68.35%   +0.04%     
==========================================
  Files           8        8              
  Lines        2584     2588       +4     
==========================================
+ Hits         1765     1769       +4     
  Misses        819      819              
Impacted Files Coverage Δ
p4runtime_sh/shell.py 67.60% <71.42%> (+0.06%) :arrow_up:
p4runtime_sh/global_options.py 89.18% <100.00%> (ø)
jafingerhut commented 1 year ago

With commit 2 on this PR, it is backwards compatible with current behavior, and provides a new optional parameter to the setup method that lets the caller disable printing.

jafingerhut commented 1 year ago

Thanks for all of the detailed suggestions. As of commit 4 on this PR, I have made changes following your 'option 2' suggestion.

I am not familiar with Python enum, so please check whether my deletion of the line @enum.unique in global_options.py looks correct. I was getting errors after adding verbose = bool with that line left in there, but did not see how else I should have fixed that.

antoninbas commented 1 year ago

Thanks for all of the detailed suggestions. As of commit 4 on this PR, I have made changes following your 'option 2' suggestion.

I am not familiar with Python enum, so please check whether my deletion of the line @enum.unique in global_options.py looks correct. I was getting errors after adding verbose = bool with that line left in there, but did not see how else I should have fixed that.

You were right to remove it. As a matter of fact, I don't know why I made this class an Enum at all, that's doesn't look right.