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

ci: add multiple test targets #159

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

eigenmannmartin
Copy link
Member

@eigenmannmartin eigenmannmartin commented Jan 17, 2025

closes ##147

@eigenmannmartin eigenmannmartin force-pushed the ci-add-multiple-test-targets branch 2 times, most recently from a4f51e5 to e542af5 Compare January 17, 2025 13:07
@eigenmannmartin eigenmannmartin force-pushed the ci-add-multiple-test-targets branch from e542af5 to 820dc75 Compare January 17, 2025 13:12
pyproject.toml Outdated
@@ -12,8 +12,8 @@ license = "MIT"
requires-python = ">=3.9"

dependencies = [
"icalendar (~=6.0)",
"python-dateutil (~=2.9)",
"icalendar (>=5.0.3)",
Copy link
Contributor

@DerDreschner DerDreschner Jan 17, 2025

Choose a reason for hiding this comment

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

That doesn't work. icalendar 6.0 had a breaking change when using the internal WINDOWS_TO_OLSON map (change of namespace) and use_pytz() needs to be used for the old behaviour as they changed the default timezone database. The latter may be not needed - I didn't tested icalevents with the new default to be honest. But I didn't want to take any risk here. See #145. It must be >=5.0.3,<6.0 here if you revert the changes from #145, but then it will not work for people who need the newest icalendar release.

I'm not that confident with Python, but I guess it's not possible to wrap the code lines in any if clause, depending on the used dependency version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, after doing some research, it may be possible. This Stack Overflow answer - together with the discussion here - would result in something like

from importlib.metadata import version

if version("icalendar") >= "6.0":
    from icalendar import use_pytz
    from icalendar.timezone.windows_to_olson import WINDOWS_TO_OLSON

    use_pytz()
else:
    from icalendar.windows_to_olson import WINDOWS_TO_OLSON

as solution. But the question is how we would test everything with both versions in the pipelines? 🤔 I can only think about two new steps using hard-coded installation actions for icalendar?

Copy link
Member

@andrewgy8 andrewgy8 Jan 19, 2025

Choose a reason for hiding this comment

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

Yep, this would be the proper work around @DerDreschner .

But the question is how we would test everything with both versions in the pipelines?

Typically you use a testing matrix which allows different versions of dependencies and python versions, like the above workflow. However, you can also add into the matrix the versions of icalendar that you would like to see tested. In this case, <6.0 and >6.0. Here is an example that uses Django in that regard and should be straightforward to add in this PR.

pyproject.toml Outdated
"icalendar (~=6.0)",
"python-dateutil (~=2.9)",
"icalendar (>=5.0.3)",
"python-dateutil (>=2.9)",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be interested to learn what the problem with ~= is in this case. 😄

@DerDreschner
Copy link
Contributor

DerDreschner commented Jan 19, 2025

I've opened a separate pull request for adding icalendar 5.0 support (#160). For housekeeping reasons, I would prefer to revert the icalendar version change and focus on the test matrix for different Python versions.

@eigenmannmartin
Copy link
Member Author

I've opened a separate pull request for adding icalendar 5.0 support (#160). For housekeeping reasons, I would prefer to revert the icalendar version change and focus on the test matrix for different Python versions.

agree!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants