refactor(gazelle): report missing BUILD_WORKSPACE_DIRECTORY key more directly

[ctcjab]

Replace os.environ.get("BUILD_WORKSPACE_DIRECTORY") with os.environ["BUILD_WORKSPACE_DIRECTORY"].

The former may return None if the environment variable is not set, in which case the code will crash with a TypeError when the line is run since the result is concatenated with a pathlib.Path object, and is therefore making it impossible to use rules_python_gazelle_plugin along with rules_mypy:

These changes allow rules_mypy users to also use rules_python_gazelle_plugin without having to work around the type error.

Now if the environment variable is not set, the code will still crash, but now with an error that better indicates the failed precondition, namely KeyError("BUILD_WORKSPACE_DIRECTORY") rather than TypeError("unsupported operand type(s) for /: 'PosixPath' and 'NoneType').

[rickeylev]

These changes allow rules_mypy users to also use rules_python_gazelle_plugin without having to work around the type error.

I don't follow. An error is still raised, so how does it help rules_mypy work better?

[rickeylev]

Thanks for the improvement!

[ctcjab]

Thanks for the improvement!

Sure thing!

I don't follow. An error is still raised, so how does it help rules_mypy work better?

In case it's still helpful, an error is still raised at runtime if the env var isn't set, but that has nothing to do with what is causing bazel build to fail here. What causes bazel build to fail is that it runs mypy on this code, and the code had a type error (it tried to concatenate a Path object with a str | None object). By rewriting the code to avoid the possible None value, it caused mypy to consider this code type safe, which in turn resolved the bazel build error. Make sense?

[ctcjab]

Hi @rickeylev, even though this was merged a month ago, I just hit this again running with the latest release of rules_python (1.6.3, from 2 weeks ago):

From https://github.com/bazel-contrib/rules_python/commits/1.6.3/gazelle/manifest/copy_to_source.py it looks like this still hasn't made it into a release version. Is there anything else I can do to help get this released?

[rickeylev]

We'll be doing a 1.7 release soon. Probably within a week.

[ctcjab]

Oh nice, thanks @rickeylev!

One follow-up thought, is it worth adding a ticket for e.g. incorporating rules_mypy into this repo's own CI, to keep new type errors from being introduced? I can create the issue if you're open to it.

[aignas]

I personally am not against doing type checking against all of the python code within the repo, but that is a fair amount of work that would definitely benefit from community also pitching in. I'll create a place-holder ticket that is reasonably wide (and we already have a CI job that is doing type checking on the runfiles library), so initially we could use something similar in our initial approach.

[ctcjab]

Thanks @rickeylev, great you're open to that. I didn't see the placholder ticket get created yet so I just created #3353 -- hope that's helpful.