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

Expose arch in Python bindings #92

Conversation

alebastr
Copy link

See #91 for the rationale. I ended up doing that myself anyways (partially, as <group arch="..." and <groupid arch="..." are yet to be implemented).

Notes:

  • I don't like translating NULL from comps_docpackage_arches to None. But tracking what else could dereference ((PyCOMPS_Sequence*)self)->list when it's set to NULL is a pain.
    The only viable alternative, IMO, is to init arches in comps_docpackage to an empty list when it's requested from the python bindings. Let me know which approach is preferred.
  • __PyCOMPS_set_arches lacks conversion from list[str]. Pretty sure I've seen the code for that in one of the source files (__pycomps_strlist_in?), but extracting and applying it may be a bit of a pain.
  • Was it even a good idea to reuse libcomps.StrSeq here? If it's the right approach, I'd prefer to also move it to a separate header+source along with get/set/conversion helpers.

Fixes #91

A few differences in the behavior may be better solved with a separate
getter/setter methods.
The default for an unset `type` was wrong and did not allow to find any
packages without explicitly passing another `type` value.
@kontura kontura self-assigned this Aug 22, 2022
@kontura
Copy link
Contributor

kontura commented Aug 25, 2022

* I don't like translating `NULL` from `comps_docpackage_arches` to `None`. But tracking what else could dereference `((PyCOMPS_Sequence*)self)->list` when it's set to `NULL` is a pain.
  The only viable alternative, IMO, is to init arches in comps_docpackage to an empty list when it's requested from the python bindings. Let me know which approach is preferred.

I would prefer the None.
Looking at the new __PyCOMPS_get_arches and __PyCOMPS_set_arches - if I am not mistaken you effectively only added the one NULL check to the getter?
I am not that familiar with the codebase but could the ids getter also benefit from this in case it would somehow get the NULL? Maybe you could just enhance the ids getter/setter.
Perhaps we could even rename them to a more general name (get/set Sequence or List?)

* `__PyCOMPS_set_arches` lacks conversion from `list[str]`. Pretty sure I've seen the code for that in one of the source files (`__pycomps_strlist_in`?), but extracting and applying it may be a bit of a pain.

I would be fine with having that but I don't think its required at this point so its up to you.

* Was it even a good idea to reuse `libcomps.StrSeq` here? If it's the right approach, I'd prefer to also move it to a separate header+source along with get/set/conversion helpers.

It seems fine, it integrates well into the existing code. Separating it would be good.

@alebastr
Copy link
Author

I would prefer the None.

Ok, I'll leave None.

Looking at the new __PyCOMPS_get_arches and __PyCOMPS_set_arches - if I am not mistaken you effectively only added the one NULL check to the getter? I am not that familiar with the codebase but could the ids getter also benefit from this in case it would somehow get the NULL? Maybe you could just enhance the ids getter/setter. Perhaps we could even rename them to a more general name (get/set Sequence or List?)

I also changed reference counting for COMPS_ObjList in the setter (Py_INCREF(value); makes the whole PyObject leak when we only need to hold a reference to the list), but that's one more thing that could (should?) be applied to the rest of the codebase.

I would be fine with having that but I don't think its required at this point so its up to you.

The write scenario looks a bit awkward without the conversion (see test changes — pkg.arches = ['x86_64'] would be more convenient), but my intended usage does not involve writes, so I'll drop the idea for now :)

* Was it even a good idea to reuse `libcomps.StrSeq` here? If it's the right approach, I'd prefer to also move it to a separate header+source along with get/set/conversion helpers.

It seems fine, it integrates well into the existing code. Separating it would be good.

Will do in the next update.

@AdamWill
Copy link

AdamWill commented Nov 2, 2022

See #91 (comment) - I'm not sure this is actually an appropriate idea, and it's not really necessary for the thing @alebastr was trying to do.

@alebastr alebastr closed this Mar 2, 2024
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.

Expose arch in Python bindings
3 participants