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.