-
Notifications
You must be signed in to change notification settings - Fork 256
Import tendril crate
#662
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
Import tendril crate
#662
Conversation
mrobinson
left a comment
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.
Thanks. I'm definitely in favor of this and archiving the tendril repository. I just have one request:
.github/workflows/tendril.yml
Outdated
| name: Tendril CI | ||
|
|
||
| on: | ||
| push: | ||
| branches: [main] | ||
| pull_request: | ||
| workflow_dispatch: | ||
| merge_group: | ||
| types: [checks_requested] | ||
|
|
||
| jobs: | ||
| linux-ci: | ||
| name: Linux | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| toolchain: ["stable", "beta", "nightly", "1.36.0"] | ||
| steps: | ||
| - uses: actions/checkout@v2 | ||
|
|
||
| - name: Install toolchain | ||
| uses: actions-rs/toolchain@v1 | ||
| with: | ||
| profile: minimal | ||
| toolchain: ${{ matrix.toolchain }} |
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.
Would it be possible to integrate this into the html5ever CI?
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.
Changes made:
- Remove
tendril.ymlworkflow - Add step for
cargo test --features 'encoding encoding_rs' - Port tendril benchmarks to criterion (which means they get picked up by html5ever's CI without any changes to the config)
I have not attempted to maintain the Rust 1.36 check as given that tendril is basically only used by html5ever, it doesn't seem useful to maintain a lower MSRV.
8fba460 to
551389b
Compare
tendril/src/stream.rs
Outdated
| #[cfg(any(feature = "encoding", feature = "encoding_rs"))] | ||
| pub type Tests = &'static [(&'static [&'static [u8]], &'static str, usize)]; | ||
|
|
||
| #[cfg(any(feature = "encoding"))] |
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.
For some reason ignoring the clippy lint for this didn't work. So I just fixed it instead.
Signed-off-by: Nico Burns <nico@nicoburns.com>
Signed-off-by: Nico Burns <nico@nicoburns.com>
Signed-off-by: Nico Burns <nico@nicoburns.com>
Signed-off-by: Nico Burns <nico@nicoburns.com>
|
This is now passing CI and ready for review. I have intentionally made as few changes as possible to the code in https://github.com/servo/tendril in this PR so as to maintain history. So I have ignored warning and clippy lints rather than fixing them, haven't updated dependencies, or migrated edition etc. Those changes can be made in follow-up PRs. We may also wish to consider rebase-merging rather than squash-merging this PR for the same reason. Once this PR is merged we should:
|
The
tendrilcrate is an important servo-owned dependency ofhtml5ever, not widely used outside ofhtml5ever, and is poorly maintained in it's current home of https://github.com/servo/tendril (which is somewhat concerning given that it contains a large amount ofunsafecode).This PR imports the
tendrilcrate into this repository where it can hopefully get more attention.