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.