♻️ Refactor the handling of shellingham

[nathanjmcdougall]

See #1297 for motivation.

There's a few places where a conditional check can be simplified but the logic is somewhat subtle. Basically the key thing to bear in mind is that the new _get_shell_name method handles the case where HAS_SHELLINGHAM is false by returning None.

There were a couple calls to shellingham.detect_shell that potentially could have raised ShellDetectionFailure, so I've put this under API ban in favour of the _get_shell_name method.

[github-actions]

📝 Docs preview for commit 81a8134 at: https://6eec02b4.typertiangolo.pages.dev

[svlandeg]

The motivation to lazy-load rich_utils was because of performance reasons, as laid out in #1128. Do you see similar performance hits for loading shellingham?

[nathanjmcdougall]

Fair question, I think probably I'm being loose with my terminology and write-ups.

Since shellingham is a conditional dependency, it's using conditional import patterns. That was true for rich even without making it lazy-loaded. In #1297, we moved to a different pattern for conditional imports using importlib instead of try-except. I do the same in this PR but for shellingham. I think there are several advantages to having a HAS_SHELLINGHAM variable instead of a possibly-None shellingham import variable (consistency with rich, type inference, monkey patching in the test suite to mock when shellingham isn't available, etc.).

The next thing the PR does is introduce a unified method _get_shell_name for detecting the shell using shellingham, with error handling for if shellingham isn't available. This adds robustness for the cases where there is an uncaught shellingham.ShellDetectionFailure error. Perhaps I ought to add test cases to prove this is the case (i.e. to prove the error no longer propogates).

A side effect of having a _get_shell_name method is that this is only one place in the codebase where shellingham is actually needed, so it makes sense to place shellingham.detect_shell under import ban to encourage the use of the _get_shell_name method instead.

What about the lazy loading? To answer your question regarding performance, I don't think performance is a concern here. However, I think it's the least confusing approach in light of the above, and it now only occurs in a single file (except the test suite, perhaps we should disable the lazy-loading Ruff rules in the test suite since it adds little value there).

[svlandeg]

Thanks for the PR @nathanjmcdougall!

I like the idea of having a HAS_SHELLINGHAM var declared in core.py just once, and importing it from there. And using find_spec instead of the try import setup we had before.

The new _get_shell_name also looks good and simplifies various other checks in the code base.

I also agree with putting shellingham.detect_shell under import ban to ensure we always use the new _get_shell_name and avoid re-raising the ShellDetectionFailure.

The only thing I don't necessarily agree with, is enforcing the "lazy loading" of shellingham in the pyproject.toml. It feels a bit unnecessarily rigid, especially if we have to rewrite the test files because of it. shellingham is specified in our requirements-tests.txt and I personally find the code less clean if the imports aren't all at the top - so I'm not in favour of those edits in the 3 test modules.

Alternatively, if we really do want to enforce using shellingham only from within functions, it would be nice if we could exclude the test suite.

[github-actions]

📝 Docs preview

Last commit 88295c1 at: https://e3d99413.typertiangolo.pages.dev

[nathanjmcdougall]

Alright, that makes sense to me. I've removed the lazy-loading aspects from the PR.

[svlandeg]

This looks good to me - I'll defer to Tiangolo for a final review though.