Skip to content

📝 Improve docstrings #112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

📝 Improve docstrings #112

wants to merge 3 commits into from

Conversation

Zethson
Copy link
Member

@Zethson Zethson commented Feb 3, 2025

Related to https://laminlabs.slack.com/archives/C07DB677JF6/p1738581253238639

Clarifies a few docstrings and tries to make them a bit more accessible.

Signed-off-by: Lukas Heumos <[email protected]>
Signed-off-by: Lukas Heumos <[email protected]>
Signed-off-by: Lukas Heumos <[email protected]>
@Zethson Zethson requested a review from falexwolf February 3, 2025 12:55
@@ -99,7 +99,7 @@ def wrapper(*args, **kwargs):
@lamin_group_decorator
@click.version_option(version=lamindb_version, prog_name="lamindb")
def main():
"""Configure LaminDB and perform simple actions."""
"""CLI for LaminDB configuration and basic operations."""
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miiiight break style but I think it's okay.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it'd be good to consistently use a verbal style across the whole docs page.

But I still think this is an improvement to the previous docstring, so let's just keep it and wait if we can come up with something more consistent and as good content wise in the future.

@@ -168,7 +168,7 @@ def init(
name: str | None,
schema: str | None,
):
"""Init an instance."""
"""Initialize an instance."""
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it when we use abbreviations such as init or specs etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use more extensive wording, we should do two things:

  1. Share the docstring with lamindb-setup (that already happens e.g. for connect() args)
  2. Look for how it's done elsewhere

I like the docstring of git init very much

image

The problem with extensive duplicated docstrings is that it will look better here but it will actually look worse when sombody goes on the docs page of ln.setup.init(). Because the discrepancy will be bigger.

So, we also have to change it here:

https://github.com/laminlabs/lamindb-setup/blob/56bc2b8a8836908783ed74f1b69091cffc7d6d60/lamindb_setup/_init_instance.py#L234

I'm using brevity for anything that's duplicated because it's easier to keep something that's brief in sync.

If you scroll a bit down you'll see that the arg definition is already shared:

https://github.com/laminlabs/lamindb-setup/blob/56bc2b8a8836908783ed74f1b69091cffc7d6d60/lamindb_setup/_init_instance.py#L237-L241

So, it'd be easy to introduce a variable DOC_INIT and populate it with the desired text and re-use it across lamindb-setup and lamin-cli.

Here is an example PR where I harmonized and improved the documentation of the lamin init args across lamindb-stup and lamin-cli


Call without subcommands and options to show settings.
Run without additional arguments to show current values.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think people call functions but run CLI commands?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say "call" because of the confusion with a "run". Otherwise good.

"""Connect to an instance.

Pass a slug (`account/name`) or URL (`https://lamin.ai/account/name`).
"""Connect to instance by slug (account/name) or URL (`https://lamin.ai/account/name`).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to share the docstring with ln.connect() as defined in lamindb-setup.

@@ -306,6 +304,9 @@ def save(filepath: str, key: str, description: str, registry: str):

You can save a `.py` or `.ipynb` file as an {class}`~lamindb.Artifact` by
passing `--registry artifact`.

To save renamed files, ensure that the tracking ID is specified in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this wording is very clear. What is a "tracking ID"? That's a word we don't use anywhere.

Best is to use an example I think as we have on the ln.track() docs.

image

Check out: https://docs.lamin.ai/lamindb.track

So, I think what we should do is somehow reuse that snippet and expose it here. Or paste a cross-reference so that people have a chance to understand what "tracking ID" (or a better word) means.

@@ -10,7 +10,7 @@

@click.group()
def cache():
"""Manage cache."""
"""Manage local file caching for lamindb."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest: "Configure the local cache of lamindb."

from lamindb_setup._cache import clear_cache_dir

clear_cache_dir()


@cache.command("get")
def get_cache():
"""Get the cache directory."""
"""Show current cache directory location."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Show the current cache directory location.

We're not omitting articles everywhere else, too.

@@ -11,20 +11,20 @@

@click.group()
def migrate():
"""Manage database schema migrations."""
"""Manage sequential database schema migrations."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the "sequential" is more confusing than helpful. And it makes it even longer.

I did a refactor of the docs a few weeks/months ago where I tried consciously introduce the term "database schema" as opposed to "dataset schema".

The main confusion lies between these two terms. If you also introduce a "random" term "sequential" it will make this less clear IMO

from lamindb_setup._migrate import migrate

return migrate.create()


@migrate.command("deploy")
def deploy():
"""Deploy migrations."""
"""Deploy pending migrations to bring database schema up to date."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit confusing IMO -- 'pending' isn't the right term, 'database schema' is also what you have in your code

I'd say "Deploy migrations to the database".

@@ -11,9 +11,9 @@
@click.group(invoke_without_command=True)
@click.pass_context
def settings(ctx):
"""Manage settings.
"""Display or modify LaminDB configuration settings.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are "configuration settings"? It's either "configuration" or "settings".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise good.

@@ -29,10 +29,10 @@ def settings(ctx):
)
@click.argument("value", type=click.BOOL)
def set(setting: str, value: bool):
"""Update settings.
"""Set LaminDB behavior flags.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's please either keep the old wording or choose something better.

"behavior flag" is to me a pretty confusing synonym for a setting 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants