load snippets location from config

[bennyweise]

📝 Summary

🔍 Description of Changes

📋 Checklist

• I have read the contributor guidelines.
• For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
• I have added tests for the changes made.
• I have run the code and verified that it works as expected.

📜 Reviewers

@akshayka OR @mscolnick

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
marimo-storybook	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 17, 2025 6:53am

[vercel]

@bennyweise is attempting to deploy a commit to the marimo Team on Vercel.

A member of the Team first needs to authorize it.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[mscolnick]

Thoughts about this being a list of strings? I could see someone asking for multiple paths

[bennyweise]

I was planning to ask you the same question - I think a list makes more sense here. Also, should the default path optional as well in the config? Something like

[tools.marimo.snippets]
use_default_path: true
custom_paths:
  - foo/bar
  - foo/baz

[mscolnick]

yea thats not a bad idea. maybe include_default_snippets with default true?

[bennyweise]

Updated no as a list of strings, with include_default_snippets as an option with default true

[mscolnick]

maybe this could be DRYed up a bit:

def _yield_py_files(root_path: str) -> Iterator[str]:
    real_path = os.path.realpath(root_path)
    for _root, _dirs, files in os.walk(real_path):
        for file in files:
            if file.endswith(".py"):
                yield os.path.join(real_path, file)

# Get all paths
default_path = str(import_files("marimo").joinpath("_snippets").joinpath("data"))
config = get_default_config_manager(current_path=None).get_config()
custom_paths: list[str] = config.get("snippets", {}).get("custom_paths", [])
if custom_paths:
    LOGGER.debug("Using custom snippets path: %s", custom_paths)
paths = [default_path] + custom_paths

for path in paths:
    yield from _yield_py_files(path)

[mscolnick]

could be more performant / easier to read using pathlib

# Get all paths
default_path = import_files("marimo") / "_snippets" / "data"
config = get_default_config_manager(current_path=None).get_config()
custom_paths = [Path(p) for p in config.get("snippets", {}).get("custom_paths", [])]
paths = [default_path] + custom_paths

# Yield from all paths
for root_path in paths:
    # TODO: maybe error catching if the path is not valid?
    for file in root_path.resolve().rglob("*.py"):
        yield str(file)

[bennyweise]

Basically conforms to this style now - thanks for the pointers

[bennyweise]

Have updated based on the comments, and tested locally with the standard test cases of real directory / non-existent directory / recursively symlinked directory, and all look ok (but I may be missing some pathological cases).

In terms of formal tests, I was wondering if there is a recommended way to test with different configs - I could patch the get_default_config method to inject various options, but that doesn't feel like a great solution. Alternatively I can extract the snippet path operations from the configuration, but I don't know that those tests would really achieve much.

[bennyweise]

I have read the CLA Document and I hereby sign the CLA

[mscolnick]

hey @bennyweise, this looks great!

I do think a test would be nice. You don't need to test change the config, etc. but maybe we can refactor the get_snippets to be more testable

def read_snippet_files(include_default_snippets: bool, custom_paths: list[str]):
    paths = []
    if include_default_snippets:
        paths.append(import_files("marimo") / "_snippets" / "data")
    if custom_paths:
        paths.extend(custom_paths)
    for root_path in paths:
        if not root_path.is_dir():
            # Note: currently no handling of permissions errors, but theoretically
            # this shouldn't be required for `is_dir` or `rglob`
            # Other possible errors:
            # - RecursionError: not possible, since by default symlinks are not followed
            # - FileNotFoundError: not possible, `is_dir` checks if the path exists,
            # but also resolve() is not called with strict=True
            LOGGER.warning(
                "Snippets path %s not a directory - ignoring", root_path
            )
            continue
        for file in root_path.resolve().rglob("*.py"):
            yield str(file)

and we can write a few different tests for

assert list(read_snippet_files(True, [])) == [..]
assert list(read_snippet_files(False, [])) == [..]
assert list(read_snippet_files(False, [good_path])) == [..]
assert list(read_snippet_files(False, [bad_path])) == [..]
assert list(read_snippet_files(False, [good_path, bad_path])) == [..]
... etc.