Handling of multiple variables of the same name in Python scripts is untested

[jaraco]

In #102, it came to my attention that some users might mistakenly add more than one __requires__ or __index_url_ or other supported variable to the globals of their script.

It's not obvious to me what pip-run should do in that case. In #102, it was proposed to silently ignore both. I think there are other options:

• silently ignore both
• honor just one (and maybe warn)
• raise an exception

In my opinion, silently ignoring both is less desirable than the other options. On one hand, it might be nice to raise an exception to let the user know they might have a mistake. On the other hand, Python doesn't warn about such a condition and always honors the most recent definition (last).

My instinct is it's probably not particularly important to worry about this concern. It seems unlikely that users will encounter this condition and so far, there have been zero reports about issues relating to it, so I'm leaning to just leaving it an untested/unsupported concern.

Also, I'll want to test to confirm what is the current behavior.

@bswck what was it to lead you to want to address this concern?

[jaraco]

Also, I'll want to test to confirm what is the current behavior.

The following test passes, so the current behavior is to silently ignore both:

diff --git a/tests/test_scripts.py b/tests/test_scripts.py
index 877c0a5..0326b4c 100644
--- a/tests/test_scripts.py
+++ b/tests/test_scripts.py
@@ -56,6 +56,15 @@ class TestSourceDepsReader:
         )
         assert scripts.DepsReader(script).read() == ['foo']

+    def test_duplicate_definition(self):
+        script = textwrap.dedent(
+            """
+            __requires__=['foo']
+            __requires__=['bar']
+            """
+        )
+        assert scripts.DepsReader(script).read() == []
+
     def test_reads_files_with_multiple_assignment(self):
         script = textwrap.dedent(
             """

[bswck]

Indeed, it passes! That's why I said #102 preserves the behavior of handling multiple assignments, and does not alter it. I saw a missing test rather as a coverage issue, not an undefined behavior.

Looking at https://github.com/jaraco/pip-run/blob/966d86b98fc889084b9fa129173f9a8616d7e60e/pip_run/scripts.py#L169-L176 if we try to unpack multiple (or no) nodes in place where one is expected (node,) a value error is raised.

Which is why maybe it's a better idea to actually make _read safe and precisely describe the cases it supports? This is what #102 tried to do, but at the cost of expanding the implementation to imperative approach.

The callee of that function just so happened to suppress ValueError, so that's why I thought the concern was addressed since the beginning.

[jaraco]

I've gone ahead and pushed that test, making it an explicitly-tested expectation.

Indeed, it passes! That's why I said #102 preserves the behavior of handling multiple assignments, and does not alter it. I saw a missing test rather as a coverage issue, not an undefined behavior.

Thanks. Yeah, that was my mis-read on the intention there.