Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/revisit get_package #297

Merged
merged 10 commits into from
Dec 20, 2020
32 changes: 14 additions & 18 deletions src/towncrier/_project.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,26 +16,22 @@

def _get_package(package_dir, package):

# Step 1: Try the dumbest and simplest thing that could possibly work.
# Yes, that means importing it. Call the cops, I don't care.

sys.path.insert(1, package_dir)

try:
module = import_module(package)
except ImportError as e:
print("Tried to import {}, but ran into this error: {}".format(package, e))
# wups that didn't work
module = None

# Don't leave trash in sys.path
sys.path.pop(0)

# Step 2: uhhhhhhh
# TBA

if not module:
raise Exception("Can't find your project :(")
except ImportError:
# Package is not already available / installed.
# Force importing it based on the source files.
sys.path.insert(0, package_dir)

try:
module = import_module(package)
except ImportError as e:
err = "tried to import {}, but ran into this error: {}".format(package, e)
# NOTE: this might be redirected via "towncrier --draft > …".
print("ERROR: {}".format(err))
raise
finally:
sys.path.pop(0)

return module

Expand Down
2 changes: 2 additions & 0 deletions src/towncrier/newsfragments/297.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
When searching for the project, first check for an existing importable instance.
This helps if the version is only available in the installed version and not the source.
60 changes: 60 additions & 0 deletions src/towncrier/test/test_project.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,66 @@ def test_tuple(self):
version = get_version(temp, "mytestproja")
self.assertEqual(version, "1.3.12")

def test_import_fails(self):
"""
An exception is raised when getting the version failed due to missing Python package files.
"""
if sys.version_info >= (3, 6):
expected_exception = ModuleNotFoundError
else:
expected_exception = ImportError

with self.assertRaises(expected_exception):
get_version(".", "projectname_without_any_files")

def test_already_installed_import(self):
"""
An already installed package will be checked before cwd-found packages.
"""
project_name = "mytestproj_already_installed_import"

temp = self.mktemp()
os.makedirs(temp)
os.makedirs(os.path.join(temp, project_name))

with open(os.path.join(temp, project_name, "__init__.py"), "w") as f:
f.write("__version__ = (1, 3, 12)")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
f.write("__version__ = (1, 3, 12)")
f.write("__version__ = "version.on.disk")

but maybe we don't need at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear what this would clarify. :]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'1.3.12' vs '1.3.13is a tiny difference :) .. .but maybe we don't even need to create these files ... and only havesys_path_temp`

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is testing that get_version() get's the version from the proper place. Unless we extracted the selecting of the package to be a separate activity (sure, that sounds maybe good) then don't we need to put versions into the packages so we can make sure the proper one was used?

'version on disk' doesn't seem to really distinguish between the two, but 'installed' vs. 'source' might? Though seems reasonable to keep them as 'versions' instead of 'names'. Perhaps (1, 3, 12) and (2, 1, 5) would make them usefully distinct so the difference is harder to overlook?


sys_path_temp = self.mktemp()
os.makedirs(sys_path_temp)
os.makedirs(os.path.join(sys_path_temp, project_name))

with open(os.path.join(sys_path_temp, project_name, "__init__.py"), "w") as f:
f.write("__version__ = (2, 1, 5)")

sys.path.insert(0, sys_path_temp)
altendky marked this conversation as resolved.
Show resolved Hide resolved
self.addCleanup(sys.path.pop, 0)

version = get_version(temp, project_name)

self.assertEqual(version, "2.1.5")

def test_installed_package_found_when_no_source_present(self):
"""
The version from the installed package is returned when there is no
package present at the provided source directory.
"""
project_name = "mytestproj_only_installed"

sys_path_temp = self.mktemp()
os.makedirs(sys_path_temp)
os.makedirs(os.path.join(sys_path_temp, project_name))

with open(os.path.join(sys_path_temp, project_name, "__init__.py"), "w") as f:
f.write("__version__ = (3, 14)")

sys.path.insert(0, sys_path_temp)
self.addCleanup(sys.path.pop, 0)

version = get_version("some non-existent directory", project_name)

self.assertEqual(version, "3.14")


class InvocationTests(TestCase):
def test_dash_m(self):
Expand Down