Additional override locations

[togakangaroo]

As per our discussion in #67

[simonrw]

Hi @togakangaroo, thanks for submitting this PR. Would you mind installing one of the Python formatters ruff or black and formatting the bin/tflocal script, since this is causing the CI to fail.

I will also review the PR after leaving this comment

[simonrw]

I have tested this change and it seems to work for my minimal example. Please fix the CI (should just require a format) and I am happy to merge this. Thanks for contributing this change 🎉

[simonrw]

Thanks for updating the documentation 🎉

[simonrw]

This is quite an obtuse line, could we perhaps split it out so that the global variable includes the raw envar (with sensible default), then parse the value on usage?

Suggested change

	ADDITIONAL_TF_OVERRIDE_LOCATIONS = (x for x in os.environ.get("ADDITIONAL_TF_OVERRIDE_LOCATIONS", default="").split(sep=",") if x and x != "")
	ADDITIONAL_TF_OVERRIDE_LOCATIONS = os.environ.get("ADDITIONAL_TF_OVERRIDE_LOCATIONS", "")

Then at the call site below:

def get_folder_paths_that_require_an_override_file() -> Iterable[str]:
    if not is_override_needed(sys.argv[1:]):
        return

    yield get_default_provider_folder_path()
    
    # handle additional override paths
    for entry in ADDITIONAL_TF_OVERRIDE_LOCATIONS.split(","):
        entry = entry.strip()
        if entry:
            yield entry

I personally prefer longer code to more sense one-liners.

Also: since you have built a generator, there is a danger of the value being read more than once, but the first iteration will exhaust the generator.

What do you think?

[togakangaroo]

Sure, I'll make the changes.

---

Since you asked, my thoughts are as follows (though again, still going to make the changes):

I am generally in the camp that when the interface is simply "Iterable", its on the consumer not to also assume that it can be enumerated multiple times especially now that type info is readily available in editors. That said, I don't feel strongly enough about it here.

The reason I would argue to put the parsing at the front is that when this script is invoked in other ways such as ipython you get a potential error message earlier rather than later. As a bonus, later on, when reading through folder path processing code you don't also have to think about parsing an environment variable in the same mental context.

I am in agreement that doing it all in one step is kinda awkward especially once you have formatters doing their thing. The problem with that in this case to me is the ALL_CAPS convention. I would want to break things up into multiple steps with intermediate variables, but the common understanding of the convention doesn't really support that. If I create a _split_additional_override_locations variable then would I then have an assignment to an ALL_CAPS variable from one that isn't? That tends to freak people out. Personally, I dislike this naming convention in general. To my view it comes from C precompilers where knowing whether a const would be substituted was really important. That doesn't really apply to languages like python which don't care where a variable value came from, so I avoid it.

But again, this is all immaterial, just since you asked. Its your project and I'll make those changes.

[togakangaroo]

Made the changes you recommended.

Could you please give it a quick test again? The eyeball test is good, but I have my localstack hosed at the moment as I use both podman and linux and going to take a weekend to fix whatever is going on in there.

[togakangaroo]

Thanks for approving @simonrw. I don't seem to have the ability to actually hit the merge button. I think you'll have to be the one to do it