Skip to main content
added 670 characters in body
Source Link
STerliakov
  • 2.1k
  • 3
  • 13

CLI

Python has a great built-in CLI arguments parser - argparse. You don't need to package it. It can pretty-print the help for you along with subcommands and flags descriptions. It will also be easier to maintain later when you decide to add some flags (-v/--version? -h/--help? --user/--system?)

  • if not dir_to_check.exists() or not dir_to_check.is_dir(): is just if not dir_to_check.is_dir(): - if it doesn't exist, it's also not a directory
  • sha256sum can indeed be trivially reimplemented in python in <10 lines, see e.g. here - that's easier than spawning a subprocess to some binary that may not be found on the system. If you decide to spawn, please at least check that it exists beforehand and die with a helpful error message suggesting to install it.
  • f.write(json.dumps(...)) should be just dumping to the file directly with json.dump(..., f)
  • Use booleans directly. if condition: flag = False; else: flag = True is a huge anti-pattern. Please prefer just flag = not condition instead.
  • You have a great docstring and some CLI help, thanks! If you ever plan to package this, please write a short manpage - that'd be really helpful for your users (well, man 1 sometool is the first instinct, sometool --help is the second, and fortunately the latter will still print help despite it being accompanied with unrecognized command notice)
  • if not dir_to_check.exists() or not dir_to_check.is_dir(): is just if not dir_to_check.is_dir(): - if it doesn't exist, it's also not a directory
  • sha256sum can indeed be trivially reimplemented in python in <10 lines, see e.g. here - that's easier than spawning a subprocess to some binary that may not be found on the system. If you decide to spawn, please at least check that it exists beforehand and die with a helpful error message suggesting to install it.
  • f.write(json.dumps(...)) should be just dumping to the file directly with json.dump(..., f)
  • Use booleans directly. if condition: flag = False; else: flag = True is a huge anti-pattern. Please prefer just flag = not condition instead.

CLI

Python has a great built-in CLI arguments parser - argparse. You don't need to package it. It can pretty-print the help for you along with subcommands and flags descriptions. It will also be easier to maintain later when you decide to add some flags (-v/--version? -h/--help? --user/--system?)

  • if not dir_to_check.exists() or not dir_to_check.is_dir(): is just if not dir_to_check.is_dir(): - if it doesn't exist, it's also not a directory
  • sha256sum can indeed be trivially reimplemented in python in <10 lines, see e.g. here - that's easier than spawning a subprocess to some binary that may not be found on the system. If you decide to spawn, please at least check that it exists beforehand and die with a helpful error message suggesting to install it.
  • f.write(json.dumps(...)) should be just dumping to the file directly with json.dump(..., f)
  • Use booleans directly. if condition: flag = False; else: flag = True is a huge anti-pattern. Please prefer just flag = not condition instead.
  • You have a great docstring and some CLI help, thanks! If you ever plan to package this, please write a short manpage - that'd be really helpful for your users (well, man 1 sometool is the first instinct, sometool --help is the second, and fortunately the latter will still print help despite it being accompanied with unrecognized command notice)
Source Link
STerliakov
  • 2.1k
  • 3
  • 13

First of all... thanks for sharing! This is a great piece of software I'd be glad to take maintenance over from you as a previous dev. Really. I will follow with quite a few notes, but they do not change the general stance: this Works™ and is in maintainable state.

Now to the problems.

Type hints

Since you're relying on type hints, please do so consistently. Return types aren't magically inferred in python, it's not typescript (and even there explicit annotations are usually preferred). Using correct annotations would have helped down the road.

E.g. in forward:

    if version is None or node_base_dir is None:
        die("No version of node available, install one using `nodeup install`")

    # This should not be possible to hit but Python's type hints are not smart
    # enough to detect that
    assert version is not None
    assert node_base_dir is not None

and in nodeup_install:

    # Yet again unlike TS, Python's type hints are not strong enough to catch
    # this is definitely not None by the time we get here which causes Mypy to
    # freak out, so we have to assert here
    assert node_rootname is not None

It's not the type system, it's missing type hints. mypy does not infer return types (and does not have to).

If die was typed correctly (below), both asserts would be unnecessary. Here's the right die signature:

from typing import Never  # `NoReturn` on python<3.11


def die(message: str | None = None) -> Never:
    ...
    # your impl

Never means "does not return", and sys.exit is exactly that kind of function.

There are other annotation problems that you will find after adding return types. lnsfT expects str but is passed Path, for example. Run mypy on your code to do the actual type checking.

Python version compat

You're writing a cross-platform, general use tool. It would be nice to support all non-EOL pythons (3.9 and newer as of now). You only use a couple of features from newer versions - PEP695 type parameter in list_get, introduced in 3.12 (has 1:1 older equivalent using typing.TypeVar) and match statement introduced in 3.10.

I can agree with match since it's only one version away from being globally available, but not with PEP695 type param - it's too new to use in a widely distributed tool.

However, if you decide to depend on this 3.12 feature, use 3.11+ contextlib.chdir as well - it does the same thing as your tmpchdir context manager.

Dependencies

Great job avoiding any third-party deps. Since you're writing a script, it's nice to not depend on any libs installed in the environment. This makes me think that pydantic is an overkill for this case, given that you only need a very simple validation.

Exception handling

You're too generous with exceptions. Bare except and except Exception are not equivalent; the former should (almost) never be used as it swalows everything including KeyboardInterrupt, the latter is a catch-all for all "regular" exceptions. Still, you only really expect a handful of them, better only catch what you need and let everything else raise a helpful error with a traceback. For example,

def should_use_symlinks():
    try:
        with open(get_nodeup_settings_file(), "r", encoding="utf-8") as f:
            return json.load(f)["use_symlinks"]
    except Exception:
        return None

This is really interested in FileNotFoundError. If there's no file, use default. But your implementation also swallows any other problem: something weird stored in the config file (e.g. [1]) or a missing key (which also means you can't trust this config - the contract is violated, someone messed up with your file!). This function will helpfully return None in any of those cases, leaving no trace to debug the problem down the road.

Validation

I see the following TODO in two places:

TODO: find the Python equivalent of TypeScript zod

There is an equivalent library, pydantic. But see Dependencies section above.

I'd rather avoid pulling in a dependency for this and slightly restructure your code. Python is great at mixing functional and OO styles, and your config operations really look like they belong to a class. Like this one (untested stub; get_nodeup_settings_file should probably also be its classmethod):

from dataclasses import dataclass, field

def _should_use_symlinks() -> bool:
    system = platform.system()
    return not ("Windows" in system or "_NT" in system)


@dataclass(kw_only=True)
class Config:
    active_node_version: str | None = None
    use_symlinks: bool = field(default_factory=_should_use_symlinks)

    def save(self) -> None:
        with open(get_nodeup_settings_file(), "w", encoding="utf-8") as f:
            json.dump(
                {
                    "active_node_version": self.active_node_version,
                    "use_symlinks": self.use_symlinks,
                },
                f,
            )

    @classmethod
    def load(cls) -> "Config":
        file = get_nodeup_settings_file()
        with open(file, "r", encoding="utf-8") as f:
            config = json.load(f)
        if not isinstance(config, dict):
            die(f"Malformed config file at {file}, expected an object at top level.")

        ver = config['active_node_version']
        if ver is not None and not isinstance(ver, str):
            die(
                f"Malformed config file at {file},"
                " expected a string or null at .active_node_version"
            )

        use_symlinks = config['use_symlinks']
        if not isinstance(use_symlinks, bool):
            die(f"Malformed config file at {file}, expected a boolean at .use_symlinks")

        return cls(active_node_version=ver, use_symlinks=use_symlinks)

    @classmethod
    def load_or_default(cls) -> "Config":
        try:
            return cls.load()
        except FileNotFoundError:
            return cls()

Even this minor refactoring (+ adding validation) can greatly reduce the code size of some functions:

def initialize_nodeup_files():
    try:
        Path.mkdir(get_nodeup_home_dir(), parents=True, exist_ok=True)
        Config().save()
        Path.mkdir(get_nodeup_node_versions_dir(), parents=True, exist_ok=True)
    except Exception:
        die("Failed to initialize nodeup files!")

def set_active_node_version(version: str):
    config = config.load_or_default()
    config.active_node_version = version
    config.save()

    if config.should_use_symlinks:
        try:
            lnsfT(
                get_nodeup_node_versions_dir() / version,
                get_nodeup_active_node_version_dir(),
            )
        except Exception:
            die("Failed to set active Node.js version!")


def get_active_node_version():
    if should_use_symlinks():  # See below regarding this branching
        try:
            return get_nodeup_active_node_version_dir().readlink().name
        except FileNotFoundError:
            return None

    return Config().load_or_default().active_node_version

Dead code

set_symlink_usage is unused. Maybe something else is too.

Code organization

In addition to extracting a Config class I already mentioned, there's one more architectural problem. A lot of your functions branch on if should_use_symlinks() and do completely different things in two branches. That means you have two sources of truth: active_node_version in config matters on some platforms and is ignored on others. This might be fine, but I'd rather stick with one source of truth (config file - it's always available) and enforce/validate that another one is in sync.

If you do that, all get_ and set_ helpers will become completely unnecessary - just load a Config and read its attribute.

Other minor notes

  • if not dir_to_check.exists() or not dir_to_check.is_dir(): is just if not dir_to_check.is_dir(): - if it doesn't exist, it's also not a directory
  • sha256sum can indeed be trivially reimplemented in python in <10 lines, see e.g. here - that's easier than spawning a subprocess to some binary that may not be found on the system. If you decide to spawn, please at least check that it exists beforehand and die with a helpful error message suggesting to install it.
  • f.write(json.dumps(...)) should be just dumping to the file directly with json.dump(..., f)
  • Use booleans directly. if condition: flag = False; else: flag = True is a huge anti-pattern. Please prefer just flag = not condition instead.