Import shared code from ansiblelint #5

ssbarnea · 2021-06-24T15:32:19Z

This change should move all the shared code from ansiblelint and allow removal of molecule dependency on ansible-lint.

Both molecule and ansible-lint with import the code from ansible-compat.

Fixes: #4

src/ansible_compat/config.py

webknjaz · 2021-06-24T16:21:02Z

src/ansible_compat/config.py

+    return Version(version)
+
+
+if ansible_collections_path() in os.environ:


It's best not to have any import-time code execution. You could use __getattr__() to make it on-access though.

If you are talking about https://www.python.org/dev/peps/pep-0562/ that is py37+, so not usable for us.

I am aware of this issue and I plan to refector the library to use a manager that can return property. That would be very useful for other bits inside too. Still, for the moment I will keep the original implementation in this PR.

There's a backport: https://pypi.org/project/pep562/

src/ansible_compat/constants.py

src/ansible_compat/prerun.py

webknjaz · 2021-06-24T16:35:34Z

src/ansible_compat/prerun.py

+
+def _update_env(varname: str, value: List[str], default: str = "") -> None:
+    """Update colon based environment variable if needed. by appending."""
+    if value:


Use a guard expression here.

Excuse my ignorance but could you show me how it should look like? (maybe even include a link an article).

Guard expressions are simple conditionals that interrupt the control flow early. Like "if X: return" or "if Y: continue/break" (for loops).

Typically, when you have something like:

if True: a huge indented block of code

you could dedent that block of code if you just invert that if-conditional expression:

if not True: return a huge indented block of code

And so this refactoring approach is called "guard expression". It allows you have less highly nested code.

webknjaz · 2021-06-24T16:36:39Z

src/ansible_compat/prerun.py

+                raise RuntimeError(f"Unexpected data read for {key}: {val}")
+            return val
+    else:
+        raise RuntimeError("Unknown data type.")


Sounds like a LookupError would be more appropriate here.

I would prefer to keep it like this as it assures that errors are nicely displayed by the consumers. Other exceptions may produce undesired stacktraces. We can debate that later but is better to avoid breaking the current "api".

This exception is too generic. It's almost the same as just using raise Exception. You just don't do this.
Besides, you're leaking the implementation details from other abstraction layers.
Normally, you need to communicate exceptions that make sense for this function and not some wrapper that may or may not be there.
Ideally, you have to raise precise narrow exceptions and convert them to other exceptions at outer levels when it makes sense. It's something the caller has enough context to decide upon, not a low-level lib deep in the call stack.

If you want compat with the old API, this could be an optional layer in this repo that some project may temporarily opt-in but it's mustn't be the default because you're shadowing what's happening. Otherwise, you make a dependency on the CLI of other projects while the dependency direction should be different. We shouldn't have cyclic deps.

webknjaz · 2021-06-24T16:38:45Z

src/ansible_compat/prerun.py

+
+    for path in paths:
+        collpath = os.path.join(path, 'ansible_collections', ns, coll)
+        if os.path.exists(collpath):


this loop has a huge potential of using guard expressions

This change should move all the shared code from ansiblelint and allow removed of molecule on ansible-lint. Both with import the code from us.

- change implementation to raise KeyError by default for missing keys - allow setting a default value, including None Follow-Up: #5 (comment)

ssbarnea added the enhancement New feature or request label Jun 24, 2021

ssbarnea force-pushed the devel branch from b8c49bd to 7c48ce8 Compare June 24, 2021 16:08