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

📝 Docs preview

Last commit 9c36709 at: https://b883b622.typertiangolo.pages.dev

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