py client version check fixes and cleanup

[melange396]

fixes on top of #1456, to address CI failures seen in #1496

[melange396]

i believe this test is now broken (or is at least inadequate) :

delphi-epidata/integrations/client/test_delphi_epidata.py

Lines 336 to 340 in 21c7df6

	@patch('delphi.epidata.client.delphi_epidata.Epidata._version_check')
	def test_version_check_once(self, version_check):
	"""Test that the _version_check() function is only called once on initial module import."""
	from delphi.epidata.client.delphi_epidata import Epidata
	version_check.assert_not_called()

[rzats]

Since _version_checked is now tracked as a class variable, it might be worth adding it to the test you've mentioned - this would verify that the state persists across repeated imports but _version_check() is only called once:

  @patch('delphi.epidata.client.delphi_epidata.Epidata._version_check')
  def test_version_check_once(self, version_check):
    """Test that the _version_check() function is only called once on initial module import."""
    from delphi.epidata.client.delphi_epidata import Epidata
    self.assertTrue(Epidata._version_checked)
    version_check.assert_not_called()

[dshemetov]

Suggested change

	# This is indended to run just once per program execution, on initial module load.
	# This is intended to run just once per program execution, on initial module load.

[dshemetov]

I was surprised that this test didn't error:

  @patch('delphi.epidata.client.delphi_epidata.Epidata._version_check')
  def test_version_check_once(self, version_check):
    """Test that the _version_check() function is only called once on initial module import."""
    from delphi.epidata.client.delphi_epidata import Epidata
    version_check.assert_not_called()

Since we moved the flag check inside the function, _version_check() should be called and then take the quick exit branch. But this doesn't error, so _version_check() was never called, which lead me to think that Python doesn't rerun a module once it's imported and this turns out to be true.

The first place checked during import search is sys.modules. This mapping serves as a cache of all modules that have been previously imported, including the intermediate paths. So if foo.bar.baz was previously imported, sys.modules will contain entries for foo, foo.bar, and foo.bar.baz. Each key will have as its value the corresponding module object.

From The module cache.

You can test it like this:

# a.py
class Test:
    @staticmethod
    def hello():
        print("Hello")

class Test2:
    pass

Test.hello()

# b.py
from a import Test
from a import Test2

➜  epidatpy git:(dev) ✗ python b.py
Hello

This holds even a is imported in different files.

So it seems like we don't actually need this flag and my bad for suggesting all this unneeded stuff. We probably don't need the unit tests associated with the check running only once either, since we don't need to test Python's import system; just test that _version_check writes the right log message and call it done. Would be good to write a comment explaining that _version_check will automatically only run on the first import.

[minhkhul]

Agree w Rostyslav on unit test. Also, just curious, what does e___vdc stands for?
Edit: just read through Dmitry's suggestions.

fulfilled w #1498

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud