-
Notifications
You must be signed in to change notification settings - Fork 100
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
chore(lib): add crate re-exports #150
Conversation
@@ -136,7 +136,13 @@ | |||
)] | |||
|
|||
#[macro_use] | |||
extern crate html5ever; | |||
pub extern crate html5ever; |
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.
Since this crate is on edition 2021 now, do we actually need the #[macro_use]
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.
hi @adamreichold yeah, we still need for now.
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 meant that we actually added the necessary use
clauses elsewhere but indeed it seems that html5ever
does not properly scope its macros, so it would probably not really make things better.
|
||
pub use cssparser; | ||
pub use ego_tree; | ||
pub use selectors; |
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.
Doesn't this one make the more selective pub use selectors::{attr::CaseSensititvy, Element};
below redundant?
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.
Pushed the update for this.
src/lib.rs
Outdated
pub use cssparser; | ||
pub use ego_tree; | ||
pub use selectors; | ||
pub use smallvec; |
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.
Re-exporting crates should be limited to those used in the public API which does not seem to be the case for at least smallvec
.
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.
Agreed, removed the smallvec
re-export.
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.
Feel free to squash the commits =).
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 started checking the others and for example, I could not find a place where tendril
is part of our API. Similarly, I don't think we expose any types from cssparser
. Could you please check that the list really only includes crates exposed in the API. Otherwise, this is an unnecessary commitment w.r.t. backwards compatibility.
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 can do this in the morning, mainly interested in the cssparser re-export, since versions of it cause breaking changes in the ‘selectors’ crate.
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.
Going to close the PR, I think it is fine to keep as is, until it becomes more adherent to extend.
No description provided.