Clean up Python code using Pylint #85
Conversation
|
|
| @@ -47,27 +49,24 @@ | |||
| SIMULATOR_COLOR = ca.Fore.BLUE + ca.Back.WHITE | |||
| RESET_COLOR = ca.Style.RESET_ALL | |||
| except: | |||
| ca = None | |||
| CA = None | |||
There was a problem hiding this comment.
Constants should be uppercase: https://www.python.org/dev/peps/pep-0008/#constants
There was a problem hiding this comment.
In general, yes, but this is a constant providing a fallback for a missing package. PEP8 itself encourages breaking other PEP8 rules if there is a functional purpose served by doing so: https://www.python.org/dev/peps/pep-0008/#id15
Here, we can check ca is None before using colorama features.
| SIMULATOR_COLOR = "" | ||
| RESET_COLOR = "" | ||
|
|
||
| try: | ||
| import qutip as qt | ||
| except: | ||
| qt = None | ||
| QT = None |
There was a problem hiding this comment.
Yes. Per pep-8 standard: https://www.python.org/dev/peps/pep-0008/#constants constants should be uppercase. This might cause a problem down the road because there won't be a reference to qt when the exception is caught, but the program was going to fail anyway because qt was None.
There was a problem hiding this comment.
Same as above, QuTiP isn't used for all features, so guarding in this way lets us generate more sensible error messages when QuTiP isn't present.
|
|
||
| try: | ||
| from IPython.display import display | ||
| except: | ||
| def display(value): | ||
| def display(): |
There was a problem hiding this comment.
Intention of this piece of code seems to be making the method silent in case the import fails. This change breaks the code if the import fails. Did you test this scenario ?
There was a problem hiding this comment.
I didn't, but i see why this wouldn't work. Procedures should not be defined with unused arguments. In my opinion, we should not catch anything if from IPython.display import display fails. Maybe put this in a class specifically for jupyter notebooks?
There was a problem hiding this comment.
I'm with @RolfHuisman, this is meant as a fallback to make sure that calls to display fail silently when run outside a Jupyter environment.
| @@ -16,6 +16,15 @@ | |||
| from abc import ABCMeta, abstractproperty | |||
| from random import randrange | |||
|
|
|||
| # CLR Imports # | |||
There was a problem hiding this comment.
While I recognize PEP8's and pylint's guidance on import orderings, we must have the append to sys.path appear before import clr, otherwise the DLLs in this module will not be found by the CLR's assembly loader.
| @@ -47,27 +49,24 @@ | |||
| SIMULATOR_COLOR = ca.Fore.BLUE + ca.Back.WHITE | |||
| RESET_COLOR = ca.Style.RESET_ALL | |||
| except: | |||
| ca = None | |||
| CA = None | |||
There was a problem hiding this comment.
In general, yes, but this is a constant providing a fallback for a missing package. PEP8 itself encourages breaking other PEP8 rules if there is a functional purpose served by doing so: https://www.python.org/dev/peps/pep-0008/#id15
Here, we can check ca is None before using colorama features.
| SIMULATOR_COLOR = "" | ||
| RESET_COLOR = "" | ||
|
|
||
| try: | ||
| import qutip as qt | ||
| except: | ||
| qt = None | ||
| QT = None |
There was a problem hiding this comment.
Same as above, QuTiP isn't used for all features, so guarding in this way lets us generate more sensible error messages when QuTiP isn't present.
|
|
||
| try: | ||
| from IPython.display import display | ||
| except: | ||
| def display(value): | ||
| def display(): |
There was a problem hiding this comment.
I'm with @RolfHuisman, this is meant as a fallback to make sure that calls to display fail silently when run outside a Jupyter environment.
| estimation_results = single_qubit_process_tomography(qsim, noise_channel, n_measurements=2000) | ||
| QSIM = qsharp.QuantumSimulator() | ||
| NOISE_CHANNEL = QSIM.get(I) | ||
| ESTIMATION_RESULTS = single_qubit_process_tomography( |
There was a problem hiding this comment.
The estimation results are definitely not a constant, and should not be treated as such.
| import System | ||
|
|
||
| # External Python Imports # | ||
|
|
||
| try: | ||
| from IPython.display import display | ||
| except ImportError: | ||
| def display(value): | ||
| def display(): |
) * Updating QDK to latest version. (microsoft#53) * Fixing doc gen call (microsoft#39) * fixing doc gen call * adding a test for docs generation * need to generate docs for source files only such that we can build the docs for muliple dlls without interferences * forgot to check if the dictionary exists before clearning it in test * Updating qdk to latest version * Fixing vs code extension build script on linux (microsoft#56) * fixing a stackoverflow for large files (microsoft#55) * Build VS Code dependencies from either pwsh or powershell. (microsoft#59) * Add automatic indentation to Visual Studio extension (microsoft#60) * Add automatic indentation to Visual Studio extension * Find the indentation of the last non-empty line The previous line can be empty, which would reset the indentation to 0 if e.g. you pressed enter twice in a row. Loop backwards to find the last non-empty line and use that line's indentation instead. * Remove assumption from GetLastNonEmptyLine * Move QsSmartIndentProvider to the top of the file * Flip order of ternary operator in GetIndentation * Add doc comment to GetDesiredIndentation * Add note about nullable return type of GetDesiredIndentation * Show signature help for the first argument (microsoft#63) We also trigger signature help on "(" * Always log exceptions from file queries in debug mode (microsoft#70) * Handling specialization redeclarations (microsoft#85) * Add install template and new project commands to VS Code extension. (microsoft#79) * Compile time errors for overflowing int and double literals (microsoft#87)
I've raised the Pylint scores a bit. I didn't do the docstrings because i wasn't sure about them. This is what we have now.