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

Add python bindings #188

Merged
merged 13 commits into from
Oct 20, 2023
Merged

Add python bindings #188

merged 13 commits into from
Oct 20, 2023

Conversation

tiltingpenguin
Copy link
Contributor

This PR adds python bindings for libeconf to be included in the library

@tiltingpenguin tiltingpenguin force-pushed the python-bindings branch 2 times, most recently from 373bc78 to e696db2 Compare October 12, 2023 13:06
@tiltingpenguin tiltingpenguin force-pushed the python-bindings branch 2 times, most recently from b95d4c9 to d5a0544 Compare October 12, 2023 13:16
@schubi2
Copy link
Collaborator

schubi2 commented Oct 13, 2023

Nice ! I am missing some docu like:

  • How to build these bindings.
  • How to use it with examples.

Testcases would also be nice. Perhaps you find a useful way for it. But docu is really required for the python libeconf user.
I have seen bindings/python3/pyproject.toml . How this will be created ?
We are using meson and cmake. Should it be included there ?

Copy link

@aplanas aplanas left a comment

Choose a reason for hiding this comment

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

Some changes, but mostly LGTM

from enum import Enum
from dataclasses import dataclass
from typing import *
from ctypes import *
Copy link

Choose a reason for hiding this comment

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

Not now. But in a future PR we should remove this and export the specific symbols.

bindings/python3/econf.py Outdated Show resolved Hide resolved
bindings/python3/econf.py Outdated Show resolved Hide resolved
bindings/python3/econf.py Outdated Show resolved Hide resolved
bindings/python3/econf.py Outdated Show resolved Hide resolved
c_key = _encode_str(key)
if not isinstance(value, float):
raise TypeError('"value" parameter must be of type float')
c_value = c_double(value)
Copy link

Choose a reason for hiding this comment

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

Do we need here some approach for a "_ensure_valid_float(value)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because floats are saved as an exponentiation so we don't have to worry about the overflow. The typecheck should be enough

Copy link

Choose a reason for hiding this comment

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

Uhmm no. The exponentiation is because IEEE 754. In any case should not be required because a Python float is a C double:

>>> sys.float_info
sys.float_info(max=1.7976931348623157e+308, max_exp=1024, max_10_exp=308, min=2.2250738585072014e-308, min_exp=-1021, min_10_exp=-307, dig=15, mant_dig=53, epsilon=2.220446049250313e-16, radix=2, rounds=1)

The max match the one that from C and C++

bindings/python3/econf.py Outdated Show resolved Hide resolved
@schubi2 schubi2 merged commit 9e9adf2 into openSUSE:master Oct 20, 2023
2 checks passed
schubi2 pushed a commit that referenced this pull request Oct 31, 2023
* add python bindings
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.

4 participants