Skip to main content
added 34 characters in body
Source Link
J_H
  • 34.9k
  • 3
  • 32
  • 129

As a command line user I would find -d (or --db) less confusing than -db which suggests it's equivalent to -d -b. Perhaps there's no need for a short form?

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.

As a command line user I would find -d (or --db) less confusing than -db. Perhaps there's no need for a short form?

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.

As a command line user I would find -d (or --db) less confusing than -db which suggests it's equivalent to -d -b. Perhaps there's no need for a short form?

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-pasta -- happens to everyone. That's why we solicit reviews.

1.0 vs v1.0
Source Link
J_H
  • 34.9k
  • 3
  • 32
  • 129

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. Some tools would unconditionally add the letter (vv1.0!), untilsome conditionally, some passed along the input unchanged. Finally they finally settled on one answer.

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.

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. Some tools would unconditionally add the letter (vv1.0!), some conditionally, some passed along the input unchanged. Finally they settled on one answer.

Source Link
J_H
  • 34.9k
  • 3
  • 32
  • 129

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.