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 TokenStream to simplify API and implementation #81
Conversation
This significantly simplifies the API, without making the implementation more unergonomic to work with. To the contrary: it becomes easier to work with since everyone knows how to work with `Vec`s. Also take this opportunity to hide `PublicItem::tokens()` behind a getter, to make the API more future-proof. Here is how this commit impacts the public API: ``` % cargo public-api --diff-git-checkouts origin/main remove-tokenstream Removed items from the public API ================================= -pub fn public_api::tokens::TokenStream::clone(&self) -> TokenStream -pub fn public_api::tokens::TokenStream::cmp(&self, other: &TokenStream) -> $crate::cmp::Ordering -pub fn public_api::tokens::TokenStream::default() -> TokenStream -pub fn public_api::tokens::TokenStream::eq(&self, other: &TokenStream) -> bool -pub fn public_api::tokens::TokenStream::extend(&mut self, tokens: impl Into<Self>) -pub fn public_api::tokens::TokenStream::fmt(&self, f: &mut $crate::fmt::Formatter<'_>) -> $crate::fmt::Result -pub fn public_api::tokens::TokenStream::fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result -pub fn public_api::tokens::TokenStream::from(token: Token) -> Self -pub fn public_api::tokens::TokenStream::from(tokens: &[Token]) -> Self -pub fn public_api::tokens::TokenStream::from(tokens: Vec<Token>) -> Self -pub fn public_api::tokens::TokenStream::is_empty(&self) -> bool -pub fn public_api::tokens::TokenStream::len(&self) -> usize -pub fn public_api::tokens::TokenStream::ne(&self, other: &TokenStream) -> bool -pub fn public_api::tokens::TokenStream::partial_cmp(&self, other: &TokenStream) -> $crate::option::Option<$crate::cmp::Ordering> -pub fn public_api::tokens::TokenStream::tokens(&self) -> impl Iterator<Item = &Token> + '_ -pub fn public_api::tokens::TokenStream::tokens_len(&self) -> usize -pub struct field public_api::PublicItem::tokens: TokenStream -pub struct field public_api::tokens::TokenStream::tokens: Vec<Token> -pub struct public_api::tokens::TokenStream Changed items in the public API =============================== (none) Added items to the public API ============================= +pub fn public_api::PublicItem::tokens(&self) -> impl Iterator<Item = &Token> ```
Hi @douweschulte! I requested a review from you. If you are not interesting in doing code review on changes like these, that is no problem at all, just let me know. I just thought it was fair to give you a chance to review this since you are the mastermind behind the token architecture :) And as always, there is no urgency. If you want to think about this for a week or whatever, that is perfectly fine. Take your time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, in another project I am working on this kind of abstraction is needed for the display implementation, but here the TokenStream
does not add anything worthwhile.
src/item_iterator.rs
Outdated
} | ||
} | ||
|
||
pub(crate) fn vec_token_to_string(tokens: &[Token]) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would propose a different name here tokens_to_string
because implementation details can be left out of names, and because you do not use Vec
here ;-).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big thanks for code review! Good catch. I forgot the rename the function after changing the parameter type. I pushed a fix for it now :)
I am a big fan of keeping things as simple as possible. This makes it easier for others to use and contribute to this library.
One thing that IMHO would simplify things is to remove
TokenStream
from both the public API and the implementation. This significantly simplifies the API, without making the implementation more unergonomic to work with. It arguably becomes easier towork with since everyone knows how to work with
Vec
s.I also took this opportunity to hide
PublicItem::tokens
behind a getter, to make the API more future-proof.Here is how this commit impacts the public API:
Here is how this impacts
cargo-public-api
: Enselic/cargo-public-api#27