It passes mypy, yay!
Nice annotations.
A small quibble, a tiny nit. Say what you mean and mean what you say.
DEFAULT_DATABASE_PATH = "./data/storage.json"
That's a lovely identifier, very informative.
Except that we're storing a str
rather than a more expressive Path.
It is helpful to the Gentle Reader to see
functions accepting a Path argument
rather than a more ambiguous str,
and helpful to the Author since
.exists() and many other convenience
methods are available.
The / slash catenation operator
is especially useful: folder / "ReadMe.txt"
I am skeptical that def mainloop() is pulling its
weight, here, but OK whatever.
The issue seems to be one of scope for app.
It is needed for the decorator usages which look very nice,
and for kicking things off.
Consider shuffling things around a bit to avoid the awkward scope detail.
Maybe def exit_console() is good?
But it, too, might be deleted.
OTOH if there's some documented scheme you're adhering to
which requires these two, great, just cite its URL so we know.
@app.command(name="init")
def initialize_database(
Consider renaming, so there's no need for the optional name parameter.
Consider deleting, as this doesn't appear to do anything.
As a command line user
I would find -d (or --db) less confusing than -db.
Perhaps there's no need for a short form?
callback=presenter.init_app,
I didn't notice a corresponding import for that reference.
is_eager=True,
I'm trying to imagine what would happen if this were set to False.
Yup, not seeing it, though the typer tutorial obliquely mentions it.
This is a slightly subtle aspect.
It would be worth a # comment,
or a unit test which highlights what goes wrong if this parameter is absent.
except OSError:
view.write_to_console("Failed to create storage.json. file", view.TextColor.ERROR)
Imagine that this triggers. The first thing a customer or maintenance engineer is going to want to know is "which error?".
The happy path is super informative: at {db_path}.
Make the sad path at least that informative.
Volunteer the details, so we don't have to repeatedly reproduce the issue.
Similarly when writing config.ini.
def initialize_configuration_file(db_path: str) -> None:
No.
This is just wrong.
Clearly it's a cfg_path, not a database.
It's bad when # comments lie,
but worse when identifiers do that.
I doubt you meant to print "Updated database path".
Just chalk it up to copy-n-pasta -- happens to everyone.
That's why we solicit reviews.
def get_version() -> str:
return f"{__app_name__} v{__version__}"
Woooo, this is kind of interesting.
Almost an existential question.
Is the version 1.0, or is it v1.0 ?
I literally dealt with that issue
in a CI/CD toolchain for more than a year
across a dozen related products,
until they finally settled on one answer.
I recommend that you not "helpfully" insert a v there.
Put a stake in the ground. Either we have (roughly) three
SemVer
numbers, or we define and document our own scheme
which is free to insist on initial v if desired.
But don't try to support a pair of related versioning schemes. Life is too short.
These modules appear to achieve the project's design goals.
I would be willing to delegate or accept maintenance tasks on this codebase.