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

Replace task macro with trait #3

Merged
merged 14 commits into from
Apr 18, 2024
Merged

Replace task macro with trait #3

merged 14 commits into from
Apr 18, 2024

Conversation

arctic-alpaca
Copy link
Member

The task trait allows users to modify all aspects of task configuration and creation. By using a trait, we also enable easy task re-use and sharing, thus allowing users to share tasks in crates.

This also removes the possibility of defining a PXROS task anywhere in the dependency tree in favor of a central point to define all tasks. This improves clarity and transparency for users.

arctic-alpaca and others added 5 commits April 4, 2024 09:54
While the macro provided good ergonomics, it hardcoded configuration parameters and hid a lot of how a task is actually created. It was also possible to create tasks from anywhere in the dependency tree without control of the PXROS application developer.
With the task trait, all configurations are user accessible and task creation is done in one single point.
A builder allows more ergonomic overriding of single parts of the configuration without providing many combinations of override methods. The current implementation prevents accidentally overriding the same configuration twice, thus making it more user-friendly and misuse-resistant.

---------

Co-authored-by: vE5li <ve5li@tuta.io>
@vE5li
Copy link
Contributor

vE5li commented Apr 4, 2024

Very nice! 👍

@daniel-veecle
Copy link
Member

Please wait my review also!

src/pxros/task.rs Outdated Show resolved Hide resolved
src/pxros/task.rs Outdated Show resolved Hide resolved
src/pxros/task.rs Outdated Show resolved Hide resolved
src/pxros/task.rs Outdated Show resolved Hide resolved
@arctic-alpaca
Copy link
Member Author

arctic-alpaca commented Apr 17, 2024

The comments should be addressed now.

I would like to add some tests, at least for the task creation config and builder. The tests are kinda blocked by PxEvents_t, PxPrio_t and similar not implementing Eq. We could of course write implementations for this, but the next version of bindgen will allow us to derive this.
To reduce work that will become redundant fast, I would suggest merging as is and creating an issue tracking the tests, so they can be added when a new version of bindgen is released.

@daniel-veecle
Copy link
Member

Very good!

@arctic-alpaca arctic-alpaca merged commit 478822c into main Apr 18, 2024
4 checks passed
@arctic-alpaca arctic-alpaca deleted the task-trait branch April 18, 2024 07:59
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