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

Package dates_calc.0.0.7 #26738

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

Conversation

rmonat
Copy link
Contributor

@rmonat rmonat commented Oct 14, 2024

dates_calc.0.0.7

A date calculation library
A date calculation library, with exact operators to add a given
number of days to a date, and approximate operators to add months or
years.



🐫 Pull-request generated by opam-publish v2.3.0

@raphael-proust
Copy link
Collaborator

Are the test failures due to the mismatch of python version?

It works on some distros (e.g., arch with python 3.12) but not on others (e.g., debian with python 3.9).

#   File "/home/opam/.opam/5.2/.opam-switch/build/dates_calc.0.0.7/_build/default/lib_python/src/dates_calc/dates.py", line 43
#     match month:
#           ^
# SyntaxError: invalid syntax

Don't know if you want to do anything about it. I think it's ok to merge and expect users to manage their python environment (although I'd recommend documenting the requirement somewhere).

@rmonat
Copy link
Contributor Author

rmonat commented Oct 15, 2024

Are the test failures due to the mismatch of python version?

It works on some distros (e.g., arch with python 3.12) but not on others (e.g., debian with python 3.9).

#   File "/home/opam/.opam/5.2/.opam-switch/build/dates_calc.0.0.7/_build/default/lib_python/src/dates_calc/dates.py", line 43
#     match month:
#           ^
# SyntaxError: invalid syntax

Don't know if you want to do anything about it. I think it's ok to merge and expect users to manage their python environment (although I'd recommend documenting the requirement somewhere).

Yes, this is pattern matching introduced by a recent Python version > 3.9. I would be in favor of merging as-is please!

@yawaramin
Copy link
Contributor

@rmonat is there any reason to require conf-python-3 for this package? The functionality of the OCaml package does not depend on Python, if I understand correctly? If an OCaml/opam user wants to use the OCaml dates_calc library, they don't really need Python, right?

If so, I would suggest removing the Python dependency for submission to opam-repository and if the user needs to use the Python version of the package, they can install it using their Python tooling which they would need to manage anyway to ensure they have the required minimum version.

@rmonat
Copy link
Contributor Author

rmonat commented Oct 21, 2024

@yawaramin thank you for your suggestion! The tests dune runs are for all languages: C, Python -- so having conf-python-3 would be nice, although we would indeed require a higher version.

@yawaramin
Copy link
Contributor

Understood, I just want to avoid dependency bloat down the line. If some packages and projects in the future use this, their entire dependency cone would be pulling in Python despite it not being needed at all. I understand that you are using dune to run the Python library tests, but it seems like we should be able to find another way instead of forcing potential users of this package to install Python.

@mseri
Copy link
Member

mseri commented Oct 22, 2024

An option could be to disable the tests here, or to make a separate package for the python tests (in the same repo) that runs only on the package CI and not here.

@rmonat
Copy link
Contributor Author

rmonat commented Oct 22, 2024

@mseri I totally agree! I removed the dependency for this release, and will do this in a future release.

Thank you for spotting this too!

"-j"
jobs
"@install"
"@runtest" {with-test}
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
"@runtest" {with-test}
# "@runtest" {with-test} # require python 3.12 to run

Copy link
Member

@mseri mseri Oct 23, 2024

Choose a reason for hiding this comment

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

To disable them but run the non-pythonic ones here, what you could do is

  • add a dates_calc_python_test.opam file, whose only purpose will be to run the python tests
    _ add (package dates_calc_python_test) to `test/dune

Then on release you only release dates_calc

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

Successfully merging this pull request may close these issues.

5 participants