From d48415af72fac842ebb0faf8a054a99dbb63ebcc Mon Sep 17 00:00:00 2001 From: Aliaksandr Yakutovich Date: Mon, 12 Dec 2022 09:14:36 +0100 Subject: [PATCH] Work on pre-commit hooks, small readme update. (#401) Changes to pre-commit setup and to the `README.md` * Remove unused .prospector.yaml * Readme: add --tag-num option to update tag number * Flake8: add more plugins to better analyse the code: * `flake8-bugbear` for detecting bugs and potential design problems * `flake8-builtins` for verifying that builtins are not used as variable names * `flake8-comprehensions` for writing better and consistent comprehensions * `flake8-debugger` for checking that there are no forgotten breakpoints * `flake8-logging-format` for consistent logging * pep8-naming `for verifying that PEP8 naming conventions are followed` * `pyflakes` for checking source files for various errors * `ryceratops` to avoid exception anti-patterns * Add nbstripout hook to strip output from Jupyter and IPython notebooks * Changes requested by the pre-commit hooks: * Apply suggestions from flake8 * Fix an exception type: ValueError->TypeError * Fix TC003 * Fix TC101 errors Co-authored-by: Daniel Hollas Co-authored-by: Jusong Yu --- .pre-commit-config.yaml | 7 +++ .prospector.yaml | 20 ------- README.md | 2 +- aiidalab_widgets_base/bug_report.py | 4 +- .../computational_resources.py | 55 +++++++++++-------- aiidalab_widgets_base/databases.py | 2 +- aiidalab_widgets_base/misc.py | 3 +- aiidalab_widgets_base/nodes.py | 20 +++---- aiidalab_widgets_base/process.py | 9 ++- aiidalab_widgets_base/structures.py | 39 ++++++------- aiidalab_widgets_base/utils/__init__.py | 14 ++--- aiidalab_widgets_base/utils/exceptions.py | 7 +++ aiidalab_widgets_base/viewers.py | 29 +++++----- aiidalab_widgets_base/wizard.py | 13 ++++- notebooks/structures.ipynb | 2 +- notebooks/wizard_apps.ipynb | 4 +- 16 files changed, 113 insertions(+), 117 deletions(-) delete mode 100644 .prospector.yaml create mode 100644 aiidalab_widgets_base/utils/exceptions.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index d5fc5c65a..b851ad1b9 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -25,6 +25,8 @@ repos: hooks: - id: flake8 args: [--count, --show-source, --statistics] + additional_dependencies: [flake8-bugbear, flake8-builtins, flake8-comprehensions, flake8-debugger, flake8-logging-format, pep8-naming, pyflakes, + tryceratops] - repo: https://github.com/pycqa/isort rev: 5.10.1 @@ -47,3 +49,8 @@ repos: hooks: - id: pyupgrade args: [--py37-plus] + + - repo: https://github.com/kynan/nbstripout + rev: 0.6.0 + hooks: + - id: nbstripout diff --git a/.prospector.yaml b/.prospector.yaml deleted file mode 100644 index 3c7cd62e3..000000000 --- a/.prospector.yaml +++ /dev/null @@ -1,20 +0,0 @@ ---- -max-line-length: 120 - -ignore-paths: - - doc - - examples - - test - - utils - -pylint: - run: true - -pyflakes: - run: false - -pep8: - run: false - -mccabe: - run: false diff --git a/README.md b/README.md index 1b28528f2..ec3a3f13f 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Hosted on [aiidalab-widgets-base.readthedocs.io](https://aiidalab-widgets-base.r ## For maintainers -To create a new release, clone the repository, install development dependencies with `pip install -e '.[dev]'`, and then execute `bumpver update [--major|--minor|--patch] [--tag [alpha|beta|rc]]`. +To create a new release, clone the repository, install development dependencies with `pip install -e '.[dev]'`, and then execute `bumpver update [--major|--minor|--patch|--tag-num] [--tag [alpha|beta|rc]]`. This will: 1. Create a tagged release with bumped version and push it to the repository. diff --git a/aiidalab_widgets_base/bug_report.py b/aiidalab_widgets_base/bug_report.py index e92056ead..55e4a35c3 100644 --- a/aiidalab_widgets_base/bug_report.py +++ b/aiidalab_widgets_base/bug_report.py @@ -111,11 +111,11 @@ def _convert_ansi_codes_to_html(msg): def _format_truncated_traceback(traceback, max_num_chars=2000): """Truncate the traceback to the given character length.""" n = 0 - for i, line in enumerate(reversed(traceback)): + for _i, line in enumerate(reversed(traceback)): n += len(_strip_ansi_codes(line)) + 2 # add 2 for newline control characters if n > max_num_chars: break - return _strip_ansi_codes("\n".join(traceback[-i:])) + return _strip_ansi_codes("\n".join(traceback[-_i:])) _ORIGINAL_EXCEPTION_HANDLER = None diff --git a/aiidalab_widgets_base/computational_resources.py b/aiidalab_widgets_base/computational_resources.py index e28cf8a2d..1085db428 100644 --- a/aiidalab_widgets_base/computational_resources.py +++ b/aiidalab_widgets_base/computational_resources.py @@ -835,14 +835,13 @@ def _configure_computer(self, computer: orm.Computer): "use_login_shell": self.use_login_shell.value, "safe_interval": self.safe_interval.value, } - if "user" in sshcfg: + try: authparams["username"] = sshcfg["user"] - else: - print( - f"SSH username is not provided, please run `verdi computer configure {self.label.value}` " - "from the command line." - ) - return False + except KeyError as exc: + message = "SSH username is not provided" + self.message = message + raise RuntimeError(message) from exc + if "proxycommand" in sshcfg: authparams["proxy_command"] = sshcfg["proxycommand"] elif "proxyjump" in sshcfg: @@ -854,19 +853,27 @@ def _configure_computer(self, computer: orm.Computer): return True + def _run_callbacks_if_computer_exists(self, label): + """Run things on an existing computer""" + try: + orm.Computer.objects.get(label=label) + for function in self._on_setup_computer_success: + function() + except common.NotExistent: + return False + else: + return True + def on_setup_computer(self, _=None): """Create a new computer.""" if self.label.value == "": # check hostname self.message = "Please specify the computer name (for AiiDA)" return False - try: - computer = orm.Computer.objects.get(label=self.label.value) + + # If the computer already exists, we just run the registered functions and return + if self._run_callbacks_if_computer_exists(self.label.value): self.message = f"A computer called {self.label.value} already exists." - for function in self._on_setup_computer_success: - function() return True - except common.NotExistent: - pass items_to_configure = [ "label", @@ -890,24 +897,24 @@ def on_setup_computer(self, _=None): ) try: computer = computer_builder.new() + self._configure_computer(computer) except ( ComputerBuilder.ComputerValidationError, common.exceptions.ValidationError, + RuntimeError, ) as err: - self.message = f"{type(err).__name__}: {err}" + self.message = f"Failed to setup computer {type(err).__name__}: {err}" return False - - try: + else: computer.store() - except common.exceptions.ValidationError as err: - self.message = f"Unable to store the computer: {err}." - return False - if self._configure_computer(computer): - for function in self._on_setup_computer_success: - function() - self.message = f"Computer<{computer.pk}> {computer.label} created" - return True + # Callbacks will not run if the computer is not stored + if self._run_callbacks_if_computer_exists(self.label.value): + self.message = f"Computer<{computer.pk}> {computer.label} created" + return True + + self.message = f"Failed to create computer {computer.label}" + return False def on_setup_computer_success(self, function): self._on_setup_computer_success.append(function) diff --git a/aiidalab_widgets_base/databases.py b/aiidalab_widgets_base/databases.py index 06ac67432..47197e977 100644 --- a/aiidalab_widgets_base/databases.py +++ b/aiidalab_widgets_base/databases.py @@ -98,7 +98,7 @@ def _on_click_query(self, change): # pylint: disable=unused-argument try: entry_cif = entry.get_cif_node() formula = entry_cif.get_ase().get_chemical_formula() - except: # noqa: E722 + except Exception: continue entry_add = ( "{} (id: {})".format(formula, entry.source["id"]), diff --git a/aiidalab_widgets_base/misc.py b/aiidalab_widgets_base/misc.py index d8ea363b9..6afbf00ac 100644 --- a/aiidalab_widgets_base/misc.py +++ b/aiidalab_widgets_base/misc.py @@ -138,9 +138,10 @@ def is_number(string): """Check if string is a number.""" try: float(string) - return True except ValueError: return False + else: + return True stack = [] stackposition = -1 diff --git a/aiidalab_widgets_base/nodes.py b/aiidalab_widgets_base/nodes.py index 35ba3fc63..0416195b2 100644 --- a/aiidalab_widgets_base/nodes.py +++ b/aiidalab_widgets_base/nodes.py @@ -73,7 +73,7 @@ class AiidaNodeTreeNode(TreeNode): def __init__(self, pk, name, **kwargs): self.pk = pk - self.nodes_registry = dict() + self.nodes_registry = {} super().__init__(name=name, **kwargs) @traitlets.default("opened") @@ -103,11 +103,9 @@ class AiidaOutputsTreeNode(TreeNode): icon = traitlets.Unicode("folder").tag(sync=True) disabled = traitlets.Bool(True).tag(sync=True) - def __init__( - self, name, parent_pk, namespaces: Tuple[str, ...] = tuple(), **kwargs - ): + def __init__(self, name, parent_pk, namespaces: Tuple[str, ...] = (), **kwargs): self.parent_pk = parent_pk - self.nodes_registry = dict() + self.nodes_registry = {} self.namespaces = namespaces super().__init__(name=name, **kwargs) @@ -173,13 +171,11 @@ def _convert_to_tree_nodes(self, old_nodes, new_nodes): @traitlets.observe("nodes") def _observe_nodes(self, change): - self._tree.nodes = list( - sorted( - self._convert_to_tree_nodes( - old_nodes=self._tree.nodes, new_nodes=change["new"] - ), - key=lambda node: node.pk, - ) + self._tree.nodes = sorted( + self._convert_to_tree_nodes( + old_nodes=self._tree.nodes, new_nodes=change["new"] + ), + key=lambda node: node.pk, ) self.update() self._refresh_output() diff --git a/aiidalab_widgets_base/process.py b/aiidalab_widgets_base/process.py index a6d3a7f86..039a3095f 100644 --- a/aiidalab_widgets_base/process.py +++ b/aiidalab_widgets_base/process.py @@ -36,6 +36,7 @@ from traitlets import Instance, Int, List, Unicode, default, observe, validate from .nodes import NodesTreeWidget +from .utils import exceptions # Local imports. from .viewers import viewer @@ -94,9 +95,9 @@ def __init__( # pylint: disable=too-many-arguments if callable(inputs_generator): self.inputs_generator = inputs_generator else: - raise ValueError( + raise TypeError( "The `inputs_generator` argument must be a function that " - f"returns input dictionary, got {inputs_generator}" + f"returns input dictionary, got {type(inputs_generator)}" ) self.disable_after_submit = disable_after_submit @@ -289,9 +290,7 @@ def follow(self, detach=False): def on_completed(self, function): """Run functions after a process has been completed.""" if self._monitor is not None: - raise RuntimeError( - "Can not register new on_completed callback functions after following has already been initiated." - ) + raise exceptions.CantRegisterCallbackError(function) self._run_after_completed.append(function) diff --git a/aiidalab_widgets_base/structures.py b/aiidalab_widgets_base/structures.py index 53a1b43f0..dd2d88fc1 100644 --- a/aiidalab_widgets_base/structures.py +++ b/aiidalab_widgets_base/structures.py @@ -33,7 +33,7 @@ # Local imports from .data import LigandSelectorWidget -from .utils import StatusHTML, get_ase_from_file, get_formula +from .utils import StatusHTML, exceptions, get_ase_from_file, get_formula from .viewers import StructureDataViewer CifData = DataFactory("core.cif") @@ -149,11 +149,8 @@ def __init__( def _structure_importers(self, importers): """Preparing structure importers.""" - if not importers: - raise ValueError( - "The parameter importers should contain a list (or tuple) of " - "importers, got a falsy object." - ) + if not isinstance(importers, (list, tuple)): + raise exceptions.ListOrTuppleError(importers) # If there is only one importer - no need to make tabs. if len(importers) == 1: @@ -163,7 +160,7 @@ def _structure_importers(self, importers): # Otherwise making one tab per importer. importers_tab = ipw.Tab() - importers_tab.children = [i for i in importers] # One importer per tab. + importers_tab.children = list(importers) # One importer per tab. for i, importer in enumerate(importers): # Labeling tabs. importers_tab.set_title(i, importer.title) @@ -480,10 +477,8 @@ def __init__(self, examples, title="", **kwargs): def get_example_structures(examples): """Get the list of example structures.""" if not isinstance(examples, list): - raise ValueError( - "parameter examples should be of type list, {} given".format( - type(examples) - ) + raise TypeError( + f"parameter examples should be of type list, {type(examples)} given" ) return [("Select structure", False)] + examples @@ -693,7 +688,7 @@ def __init__(self, title=""): except ImportError: self.disable_openbabel = True - try: + try: # noqa: TC101 from rdkit import Chem # noqa: F401 from rdkit.Chem import AllChem # noqa: F401 except ImportError: @@ -1210,13 +1205,13 @@ def def_point(self, _=None): """Define the action point.""" self.point.value = self.vec2str(self.sel2com()) if self.autoclear_selection.value: - self.selection = list() + self.selection = [] def def_axis_p1(self, _=None): """Define the first point of axis.""" self.axis_p1.value = self.vec2str(self.sel2com()) if self.autoclear_selection.value: - self.selection = list() + self.selection = [] def def_axis_p2(self, _=None): """Define the second point of axis.""" @@ -1234,7 +1229,7 @@ def def_axis_p2(self, _=None): ) self.axis_p2.value = self.vec2str(com) if self.autoclear_selection.value: - self.selection = list() + self.selection = [] def def_perpendicular_to_screen(self, _=None): """Define a normalized vector perpendicular to the screen.""" @@ -1373,9 +1368,9 @@ def mod_element(self, _=None, atoms=None, selection=None): lgnd = initial_ligand.copy() lgnd.translate(position) atoms += lgnd - new_selection = [ - i for i in range(last_atom, last_atom + len(selection) * len(lgnd)) - ] + new_selection = list( + range(last_atom, last_atom + len(selection) * len(lgnd)) + ) self.structure, self.selection = atoms, new_selection @@ -1390,7 +1385,7 @@ def copy_sel(self, _=None, atoms=None, selection=None): add_atoms.translate([1.0, 0, 0]) atoms += add_atoms - new_selection = [i for i in range(last_atom, last_atom + len(selection))] + new_selection = list(range(last_atom, last_atom + len(selection))) self.structure, self.selection = atoms, new_selection @@ -1423,9 +1418,7 @@ def add(self, _=None, atoms=None, selection=None): atoms += lgnd - new_selection = [ - i for i in range(last_atom, last_atom + len(selection) * len(lgnd)) - ] + new_selection = list(range(last_atom, last_atom + len(selection) * len(lgnd))) self.structure, self.selection = atoms, new_selection @@ -1435,4 +1428,4 @@ def remove(self, _, atoms=None, selection=None): """Remove selected atoms.""" del [atoms[selection]] - self.structure, self.selection = atoms, list() + self.structure, self.selection = atoms, [] diff --git a/aiidalab_widgets_base/utils/__init__.py b/aiidalab_widgets_base/utils/__init__.py index 6390981b3..69d518986 100644 --- a/aiidalab_widgets_base/utils/__init__.py +++ b/aiidalab_widgets_base/utils/__init__.py @@ -36,14 +36,14 @@ def predefine_settings(obj, **kwargs): raise AttributeError(f"'{obj}' object has no attribute '{key}'") -def get_ase_from_file(fname, format=None): # pylint: disable=redefined-builtin +def get_ase_from_file(fname, file_format=None): # pylint: disable=redefined-builtin """Get ASE structure object.""" # store_tags parameter is useful for CIF files # https://wiki.fysik.dtu.dk/ase/ase/io/formatoptions.html#cif - if format == "cif": - traj = read(fname, format=format, index=":", store_tags=True) + if file_format == "cif": + traj = read(fname, format=file_format, index=":", store_tags=True) else: - traj = read(fname, format=format, index=":") + traj = read(fname, format=file_format, index=":") if not traj: raise ValueError(f"Could not read any information from the file {fname}") return traj @@ -80,13 +80,13 @@ def string_range_to_list(strng, shift=-1): singles = [int(s) + shift for s in strng.split() if s.isdigit()] ranges = [r for r in strng.split() if ".." in r] if len(singles) + len(ranges) != len(strng.split()): - return list(), False + return [], False for rng in ranges: try: start, end = rng.split("..") singles += [i + shift for i in range(int(start), int(end) + 1)] except ValueError: - return list(), False + return [], False return singles, True @@ -102,7 +102,7 @@ def get_formula(data_node): elif isinstance(data_node, CifData): return data_node.get_ase().get_chemical_formula() else: - raise ValueError(f"Cannot get formula from node {type(data_node)}") + raise TypeError(f"Cannot get formula from node {type(data_node)}") class PinholeCamera: diff --git a/aiidalab_widgets_base/utils/exceptions.py b/aiidalab_widgets_base/utils/exceptions.py new file mode 100644 index 000000000..452419c76 --- /dev/null +++ b/aiidalab_widgets_base/utils/exceptions.py @@ -0,0 +1,7 @@ +class ListOrTuppleError(TypeError): + """Raised when the provided value is not a list or a tupple.""" + + def __init__(self, value): + super().__init__( + f"The provided value '{value}' is not a list or a tupple, but a {type(value)}." + ) diff --git a/aiidalab_widgets_base/viewers.py b/aiidalab_widgets_base/viewers.py index c7f96d0a1..5052983da 100644 --- a/aiidalab_widgets_base/viewers.py +++ b/aiidalab_widgets_base/viewers.py @@ -46,7 +46,7 @@ from .misc import CopyToClipboardButton, ReversePolishNotation from .utils import ase2spglib, list_to_string_range, string_range_to_list -AIIDA_VIEWER_MAPPING = dict() +AIIDA_VIEWER_MAPPING = {} def register_viewer_widget(key): @@ -72,7 +72,6 @@ def viewer(obj, downloadable=True, **kwargs): try: _viewer = AIIDA_VIEWER_MAPPING[obj.node_type] - return _viewer(obj, downloadable=downloadable, **kwargs) except (KeyError) as exc: if obj.node_type in str(exc): warnings.warn( @@ -80,7 +79,9 @@ def viewer(obj, downloadable=True, **kwargs): "itself.".format(type(obj)) ) return obj - raise exc + raise + else: + return _viewer(obj, downloadable=downloadable, **kwargs) class AiidaNodeViewWidget(ipw.VBox): @@ -262,7 +263,7 @@ def _selection_tab(self): # clear_selection.on_click(lambda _: self.set_trait('selection', list())) # lambda cannot contain assignments clear_selection.on_click( lambda _: ( - self.set_trait("selection", list()), + self.set_trait("selection", []), self.set_trait("selection_adv", ""), # self.wrong_syntax.layout.visibility = 'hidden' ) @@ -531,14 +532,14 @@ def _render_structure(self, change=None): bonds = [] - cutOff = neighborlist.natural_cutoffs( + cutoff = neighborlist.natural_cutoffs( bb ) # Takes the cutoffs from the ASE database - neighborList = neighborlist.NeighborList( - cutOff, self_interaction=False, bothways=False + neighbor_list = neighborlist.NeighborList( + cutoff, self_interaction=False, bothways=False ) - neighborList.update(bb) - matrix = neighborList.get_connectivity_matrix() + neighbor_list.update(bb) + matrix = neighbor_list.get_connectivity_matrix() for k in matrix.keys(): i = bb[k[0]] @@ -659,7 +660,7 @@ def highlight_atoms( ) # pylint:disable=protected-access self._viewer.add_ball_and_stick( # pylint:disable=no-member name="selected_atoms", - selection=list() if vis_list is None else vis_list, + selection=[] if vis_list is None else vis_list, color=color, aspectRatio=size, opacity=opacity, @@ -671,7 +672,7 @@ def _default_supercell(self): @default("selection") def _default_selection(self): - return list() + return [] @validate("selection") def _validate_selection(self, provided): @@ -787,8 +788,8 @@ def _valid_structure(self, change): # pylint: disable=no-self-use if isinstance(structure, Node): self.pk = structure.pk return structure.get_ase() - raise ValueError( - "Unsupported type {}, structure must be one of the following types: " + raise TypeError( + f"Unsupported type {type(structure)}, structure must be one of the following types: " "ASE Atoms object, AiiDA CifData or StructureData." ) @@ -811,7 +812,7 @@ def _update_structure_viewer(self, change): comp_id ) in self._viewer._ngl_component_ids: # pylint: disable=protected-access self._viewer.remove_component(comp_id) - self.selection = list() + self.selection = [] if change["new"] is not None: self._viewer.add_component(nglview.ASEStructure(change["new"])) self._viewer.clear() diff --git a/aiidalab_widgets_base/wizard.py b/aiidalab_widgets_base/wizard.py index 21f2fc30b..7202fe68b 100644 --- a/aiidalab_widgets_base/wizard.py +++ b/aiidalab_widgets_base/wizard.py @@ -12,6 +12,15 @@ import traitlets +class AtLeastTwoStepsWizardError(ValueError): + """Using WizardAppWidget only makes sense if the number of setps is at least two.""" + + def __init__(self, steps): + super().__init__( + f"The number of steps of a WizardAppWidget must be at least two, but {len(steps)} were provided." + ) + + class WizardAppWidgetStep(traitlets.HasTraits): "One step of a WizardAppWidget." @@ -93,9 +102,7 @@ def __init__(self, steps, **kwargs): # The number of steps must be greater than one # for this app's logic to make sense. if len(steps) < 2: - raise ValueError( - "The number of steps of a WizardAppWidget must be at least two." - ) + raise AtLeastTwoStepsWizardError(steps) self.steps = steps diff --git a/notebooks/structures.ipynb b/notebooks/structures.ipynb index 2b4684cd1..62a56355d 100644 --- a/notebooks/structures.ipynb +++ b/notebooks/structures.ipynb @@ -79,7 +79,7 @@ "name": "python", "nbconvert_exporter": "python", "pygments_lexer": "ipython3", - "version": "3.7.9" + "version": "3.9.13" } }, "nbformat": 4, diff --git a/notebooks/wizard_apps.ipynb b/notebooks/wizard_apps.ipynb index 98e694679..1439f4fbb 100644 --- a/notebooks/wizard_apps.ipynb +++ b/notebooks/wizard_apps.ipynb @@ -14,9 +14,7 @@ "cell_type": "code", "execution_count": null, "id": "thirty-catholic", - "metadata": { - "scrolled": false - }, + "metadata": {}, "outputs": [], "source": [ "import enum\n",