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

SWIG: drop the std::optional from the public API #217

Merged
merged 7 commits into from
Feb 7, 2023

Conversation

jan-kolarik
Copy link
Member

SWIG bindings don't support the std::optional type wrappers, so this PR aims to drop all these references away from the public API.

Closes #186.

@jan-kolarik
Copy link
Member Author

Note: CI tests are failing due to API change in binding. This is resolved in related PR rpm-software-management/ci-dnf-stack#1200.

@jan-kolarik jan-kolarik force-pushed the jkolarik/swig-public-api-fixes branch from 819b3f3 to 4bb56c3 Compare January 4, 2023 14:53
@jan-kolarik jan-kolarik force-pushed the jkolarik/swig-public-api-fixes branch from 4bb56c3 to 3c36619 Compare January 13, 2023 13:54
@mcurlej mcurlej added this to the Fedora 38 milestone Jan 18, 2023
@j-mracek j-mracek self-assigned this Jan 24, 2023
@jan-kolarik jan-kolarik force-pushed the jkolarik/swig-public-api-fixes branch 2 times, most recently from f02e9c3 to 692bc79 Compare January 26, 2023 15:32
As SWIG bindings don't support std::optional wrappers, moving to default empty string value as an alternative.
As SWIG bindings don't support std::optional wrappers, moving the GoalJobSettings fields into private part of the class.
As SWIG bindings don't support std::optional wrappers, moving to default empty string parameter value as an alternative.
As SWIG bindings don't support std::optional wrappers, moving the fields into private part of the class and switch to raw pointer getters.
As SWIG bindings don't support std::optional wrappers, moving to raw pointer getter.
As SWIG bindings don't support std::optional wrappers, switching the run() method parameters to have default values instead.

Also parameters reordering was done, because more common usecase is not passing the user_id parameter.
Expose `advisory_filter` and `group_package_types` through getters on the `GoalJobSettings` struct.
@jan-kolarik jan-kolarik force-pushed the jkolarik/swig-public-api-fixes branch from 692bc79 to 393f819 Compare January 26, 2023 15:39
@j-mracek
Copy link
Contributor

j-mracek commented Feb 2, 2023

LGTM

@j-mracek
Copy link
Contributor

j-mracek commented Feb 7, 2023

@jan-kolarik It looks like that some test is failing. May I ask you to investigate the issue.

@jan-kolarik
Copy link
Member Author

@jan-kolarik It looks like that some test is failing. May I ask you to investigate the issue.

Yes, these are the API bindings tests, because of the change in the API. There is a PR mentioned in the previous comment fixing this (rpm-software-management/ci-dnf-stack#1200).

@j-mracek
Copy link
Contributor

j-mracek commented Feb 7, 2023

LGTM

@j-mracek j-mracek merged commit 3092cd6 into main Feb 7, 2023
@j-mracek j-mracek deleted the jkolarik/swig-public-api-fixes branch February 7, 2023 14:22
@j-mracek
Copy link
Contributor

j-mracek commented Feb 7, 2023

Thank you very much for your contribution

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

Successfully merging this pull request may close these issues.

SWIG: drop std::optional from the public API
5 participants