Impl/create actions #3
Conversation
.github/workflows/deploy.yml
Outdated
There was a problem hiding this comment.
this will auto-publish the crate to crates.io any time you publish a new tag. just need to setup your token in the repository's secrets. see this blogpost for more detail, or send me an email and i'd be happy to help get this setup.
| - run: cargo clippy --tests # -- -Dclippy::all -Dclippy::pedantic | ||
| - run: cargo clippy --all-features --tests # -- -Dclippy::all -Dclippy::pedantic |
There was a problem hiding this comment.
i commented out these flags that make clippy stricter. things pass right now with the default flags, but let me know if you have any preferences for the settings to use here.
| # this fails right now because of errors in get-size-derive. | ||
| # number-general should require 0.1.3 | ||
| # minimal: | ||
| # name: Minimal versions | ||
| # runs-on: ubuntu-latest | ||
| # timeout-minutes: 45 | ||
| # steps: | ||
| # - uses: actions/checkout@v4 | ||
| # - uses: dtolnay/rust-toolchain@nightly | ||
| # - run: cargo generate-lockfile -Z minimal-versions | ||
| # - run: cargo check --locked |
There was a problem hiding this comment.
enabling this test would require fixing some version requirements in some of the upstream crates, specifically updating number-general to require v0.1.3 of get-size-derive. maybe more, but that's where this broke and I didn't go any further with testing it.
| # fuzz testing async code seems hard | ||
| # fuzz: | ||
| # name: Fuzz | ||
| # runs-on: ubuntu-latest | ||
| # timeout-minutes: 45 | ||
| # steps: | ||
| # - uses: actions/checkout@v4 | ||
| # - uses: dtolnay/rust-toolchain@nightly | ||
| # - uses: dtolnay/install@cargo-fuzz | ||
| # - run: cargo fuzz check |
There was a problem hiding this comment.
idk if you wanna eventually get fuzz testing to work, but doing it async is a bit more of a lift than i wanted to get into for this PR
Cargo.toml
Outdated
There was a problem hiding this comment.
these version bumps shouldn't change much, but were just made to start satisfying requirements of the minimal check, i.e.
cargo generate-lockfile -Z minimal-versions
cargo check --locked
| } else if self.buffer.len() >= FALSE.len() && self.buffer.starts_with(FALSE) { | ||
| self.decode_bool(visitor).await | ||
| } else if self.buffer.len() >= TRUE.len() && self.buffer.starts_with(TRUE) { |
There was a problem hiding this comment.
would maybe want to make sure that a test hits this, but clippy essentially complained that since the two conditional blocks are the same, the conditions should just be merged, so that's what i did here. the logic should be identical though.
There was a problem hiding this comment.
i could be wrong, but after trying to set some breakpoints and running the existing tests, it doesn't look like anything hits this condition.
before this gets merged, i'll update one of the tests so that these lines have coverage!
There was a problem hiding this comment.
updated this test here, and confirmed it hits this code path.
| match ready!(this.value.as_mut().poll_next(cxt)) { | ||
| Some(result) => Some(result), | ||
| None => None, | ||
| } |
There was a problem hiding this comment.
this match doesn't actually do anything, since the output of ready! is already either None or Some(result)
| struct Error; | ||
|
|
||
| impl<'en> IntoStream<'en> for Error { | ||
| fn into_stream<E: Encoder<'en>>(self, encoder: E) -> Result<E::Ok, E::Error> { | ||
| "an error!".into_stream(encoder) | ||
| } | ||
| } |
There was a problem hiding this comment.
clippy complained that this was unused, so i got rid of it. is that accurate? is it used anywhere?
|
okay @haydnv this is ready for a review! most of the open threads are just a heads up / pointing something out, so feel free to resolve them as you see fit. let me know if there's anything you'd like to add/remove etc. hope this is helpful! |
let me know if you have any thoughts - happy to add/remove any tests that you do or don't want, as well as adjust any to your preferences as far as flags and whatnot. i figured serde was a good starting place, and we can make adjustments as necessary.
what the PR changes
this PR adds two actions,
test.ymlanddeploy.ymltest.ymlis copied from serde's ci.ymldeploy.ymlis copied from this blog post, it pushes a new version to crates.io whenever you publish a new tag on GitHub.it also makes some small changes, mostly to get clippy's approval.
test.ymltwo of the checks i copied over are checking for clippy warnings and minimal version checks
clippy
pedantic, as serde has. let me know if you have preferences thereminimal-versionsi made some of the version requirements slightly stricter (e.g. requiring at least v0.1.78 of async-trait rather than just v0.1), as a result of the
minimalcheck intest.yml, but ended up commenting that out since some dependencies withinnumber-generalcause it to fail. if/when we want to add that in, we can add these checks to those repositories as well in order to get that passing.fuzz testing
fuzz testing async code seems tough to setup, so that's commented out for now.
running actions
i'm not sure if there's something you have to approve in order to let the actions run, but you can see them passing (and a bit more detail) in my initial PR into my own repo