Conversation
zeenix
force-pushed
the
cursor
branch
2 times, most recently
from
e31ac25 to
35adbdf
Compare
|
@spookyvision may I ask what makes you 👎 this PR? If it's the use of LLM, perhaps you would have felt fine about this if I hadn't followed the guidelines of the Rust embedded community and hadn't disclosed the use of LLM? |
zeenix
force-pushed
the
cursor
branch
2 times, most recently
from
e766bae to
a61f122
Compare
|
I believe I've now addressed all the comments (except for the very unhelpful 👎 reactions from @spookyvision and @jamwaffles, which I can't do anything about). @Dirbaio Any chance you'd be able to review soon? |
|
@reitermarkus @MabezDev Thanks so much! I rebased the branch on latest master now. |
@MabezDev While I think that's fair, I think we also shouldn't block things on @Dirbaio. They have single-handedly created most of this amazing Rust embedded world after all and they are one person in the end. Moreover, I have pinged them many times over the months about this PR, including after your comment on the Matrix channel and here, so it's safe to say that they don't have time for this right now. So I'd humbly request that this be merged, unless @Dirbaio raises an objection/cocern. It has received multiple reviews and IMO a pretty straight forward PR. |
|
Should this be on embedded-io or on embedded-io-adapters? If it's in embedded-io we can never ever make breaking changes to it once released. (I believe we've already discussed this in Matrix?) |
|
There's some divergence compared to std cursor:
|
P.S. I'm AFK for a week so I'll address your concerns when I'm back. |
it wasn't removed, it's still there |
IMO it belongs here (embedded-io) but I don't foresee any breaking changes in this API. Perhaps that's why it's probably best we discuss this API to death to ensure all is good. :) However, if you or other maintainers think that it's best in the adapters crate, I'm happy to move it.
Ah right. Sorry, I was already on holidays brain. 😆 Since you're the second person to say this here, I guess we can remove it, yeah. |
generally we've tried to keep the trait crates as traits only. The adapters could also be in embedded-io but they're in a separate crate due to this. We can discuss on Tue in the meeting. |
All addressed, I believe. |
Oh, turns out there was a reason for this: our MSRV. |
This is a no_std variant of `std::io::Cursor`. Fixes rust-embedded#631. The implementation was generated by an LLM by copying the std impl and adapting it. I (Zeeshan) reviewed the code and adjusted small bits. The tests added are further guarantee that the code is doing what it's supposed to do.
I made them non-const again for now. Let me know if you'd rather bump the MSRV and have these const. I also took the liberty of refactoring a bit to avoid code duplication in all the writing impls. |
|
i'd just bump the MSRV yes |
8 participants
This is a no_std variant of
std::io::Cursor.Fixes #631.
The implementation was generated by an LLM by copying the std impl and adapting it. I (Zeeshan) reviewed the code and adjusted small bits. The tests added are a further assurance that the code is doing what it's supposed to do. Of course, it needs to be still reviewed by at least another person before we can be 100% certain about it.