Add unique filter#405
Conversation
9475376 to
6fd12db
Compare
|
The filter must be added to the I didn't come up with a better implementation. I tried if it was possible to insert the element into a (Also, it's odd that |
| let mut set = HashSet::new(); | ||
|
|
||
| it.into_iter().filter_map(move |elem| { | ||
| // To prevent cloning the data, we need to use `Rc`, like that we can clone `elem` as | ||
| // key of the `HashSet` and return it. | ||
| let elem = Rc::new(elem); | ||
|
|
||
| if set.insert(Rc::clone(&elem)) { | ||
| Some(elem) | ||
| } else { | ||
| None | ||
| } | ||
| }) |
There was a problem hiding this comment.
If we increase the msrv to 1.83, then we could use VacantEntry::insert_entry(). This way the Rc only needs to be cloned if the key is actually absent from the list ↓. What do you think? Would it be worth it?
let mut set = HashMap::new();
it.into_iter().filter_map(move |elem| {
// To prevent cloning the data, we need to use `Rc`, like that we can clone `elem` as
// key of the `HashSet` and return it.
if let Entry::Vacant(entry) = set.entry(Rc::new(elem)) {
Some(Rc::clone(entry.insert_entry(()).key()))
} else {
None
}
})There was a problem hiding this comment.
Huuuuuum... Ok let's go!
I tried a lot of approaches, even to have a struct containing the hashmap and the iterator etc. But performance wise, it was always terrible hence why I used And yep, it's odd but I think it's to keep only the minimum necessary inside |
6fd12db to
fc3fcca
Compare
|
It should fail CI this time. |
595cbbf to
34b3297
Compare
|
Seems like the version wasn't updated in the fuzzer but can't figure out where the rust version is defined for it... Any idea @Kijewski ? |
|
Hm, I didn't find a switch for that. Let's see if adding a "rust-toolchain.toml" works: 27f7357 |
40306f0 to
7bab174
Compare
|
Looks like set change has to be made in oss-fuzz:
I'll open a PR. |
The next version of askama (v0.14) will need at least rust 1.83 to compile. The current toolchain shipped in docker is rust 1.81, so the project could not get fuzzed anymore. This PR sets the used toolchain to 1.83. Cc @GuillaumeGomez Related issue: <askama-rs/askama#405>
|
Looks like google/oss-fuzz#13237 worked! Needs rebasing because of #403. |
34b3297 to
5e572eb
Compare
| /// | ||
| /// assert_eq!( | ||
| /// Example { example: vec!["a", "b", "a", "c"] }.to_string(), | ||
| /// "a,b,c" |
There was a problem hiding this comment.
This test is supposed to fail btw.
There was a problem hiding this comment.
Look like my usage of cargo nextest is wrong. Because of you I think using nextest is not needed anymore. :) You could please try if replacing cargo nextest run with cargo test in the CI config causes a failure as it should?
There was a problem hiding this comment.
Oops, I forgot about that before merging. :/
There was a problem hiding this comment.
Hi, sorry for jumping in.
nextest does not support doctests nextest-rs/nextest#16
What people usually do (and what nextest maintainers recommend at the issue) is to run nextest as you did before in CI, and add another step just for doctests with cargo test --doc
| - uses: Swatinem/rust-cache@v2 | ||
| - run: cargo build --all-targets --features full | ||
| - run: cargo nextest run --all-targets --no-tests=warn --features full | ||
| - run: cargo test run --all-targets --no-tests=warn --features full |
77e6b07 to
5326c51
Compare
5326c51 to
429fa4f
Compare
The next version of askama (v0.14) will need at least rust 1.83 to compile. The current toolchain shipped in docker is rust 1.81, so the project could not get fuzzed anymore. This PR sets the used toolchain to 1.83. Cc @GuillaumeGomez Related issue: <askama-rs/askama#405>
Part of the missing jinja filters.
Implementation-wise, it's the best I could come up with.