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

Remove excess crates #102

Merged
merged 8 commits into from
Jul 5, 2023
Merged

Remove excess crates #102

merged 8 commits into from
Jul 5, 2023

Conversation

TTWNO
Copy link
Member

@TTWNO TTWNO commented Jul 2, 2023

No description provided.

* atspi-macros was only required for the "interesting" functionality in atspi-client
	* This was only enabled with the `unstable_traits` flag.
	* It was a previous attmept to add universal functions to anything which shares the same trait as "Accessible".
	* the `unstable_traits` flag would define a trait, "Accessible", and implement it for AccessibleProxy.
	* Unfortunately, this ended up being a very clunky API.
	* And was unused outside of Odilia.
	* This functionality is better served by `_ext` crates, or future abstractions that are more obvious.
* Time to remove this mess, and just make long-term maintainence simpler.
@codecov
Copy link

codecov bot commented Jul 2, 2023

Codecov Report

Merging #102 (521b53c) into main (c0ba51b) will increase coverage by 18.68%.
The diff coverage is 99.18%.

@@             Coverage Diff             @@
##             main     #102       +/-   ##
===========================================
+ Coverage   69.88%   88.56%   +18.68%     
===========================================
  Files          67       40       -27     
  Lines        5217     2983     -2234     
===========================================
- Hits         3646     2642     -1004     
+ Misses       1571      341     -1230     
Impacted Files Coverage Δ
atspi-proxies/src/lib.rs 100.00% <ø> (ø)
atspi-common/src/state.rs 85.52% <99.16%> (+11.36%) ⬆️
atspi-common/src/events/object.rs 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@TTWNO
Copy link
Member Author

TTWNO commented Jul 2, 2023

We have way more test coverage than we thought. This code was a selfish addition to the core of atspi that was used only for Odilia. Generally, I'm trying to focus on things which are easy to justify for all, and not just for Odilia.
This is the same reason I closed #91 , since this would be introducing a library feature for a client. If Odilia needs Serialized errors, that's on Odilia.

Hopefully everyone is good with this. It should bring down download time for anyone using the libraries, since two entire crates have been removed. The only disadvantage for clients is the removal of the Convertable trait, which allowed a fallible conversion from various interfaces based on what interfaces were implemented.

If anybody thinks that would still be useful, I will add it back in under atspi-proxies, since it's a relatively small, simple addition. @luukvanderduim , thoughts?

@TTWNO TTWNO linked an issue Jul 2, 2023 that may be closed by this pull request
Copy link
Collaborator

@luukvanderduim luukvanderduim left a comment

Choose a reason for hiding this comment

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

I must confess I feel guilty over this removal because I started a refactor on AccessibiltyExt and full MatchRule object search in a remote object tree but got stuck at ~80% because I had trouble understanding what it was we abstracted over, especially regarding the Error types.

I suspect we chose sub optimal abstraction with respect to our own ergonomics.

That being said, We should offer the ability to abstract over interface traits without having an intrusive feature to our core crates.

Having some sort of atspi-utilities crate somewhere I think is desirable, users may find that convenient.

Let's remove this for now and explore how to add marker traits to interface types - and make sure one could extend externally by abstracting over those.

atspi-common/src/state.rs Show resolved Hide resolved
@TTWNO TTWNO merged commit 06c3943 into main Jul 5, 2023
@TTWNO TTWNO deleted the remove-excess-crates branch July 5, 2023 19:42
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.

Remove atspi-macros
2 participants