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[bot]]

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

Name	Status	Preview	Comments	Updated (UTC)
marimo-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 20, 2025 3:53pm
marimo-storybook	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 20, 2025 3:53pm

[vercel[bot]]

@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[bot]]

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

[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.

[bennyweise]

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.

Great, I've added some tests for the standard cases. There are some edge cases that may or may not be useful to resolve - e.g. you will end up with duplicate snippets if the default directory is listed in the custom paths. Is this something I should address with this change?

[mscolnick]

you will end up with duplicate snippets if the default directory is listed in the custom paths.

@bennyweise, I think this is ok to ignore for now.

code/tests look great to me!

[github-actions[bot]]

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.10.15-dev16