Skip to content
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.

Sign up for GitHub

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

Jump to bottom

With capacity #40

Merged
merged 13 commits into from
Sep 19, 2023
Merged

With capacity #40

merged 13 commits into from
Sep 19, 2023

Conversation

chris-ha458
Copy link
Contributor

this implements both
Counter::with_capacity and Counter::init_with_capacity with the bare minimum tests

i tested internally but atleast the tests pass even if i replace init with init_with_capacity but I didn't want to make drastic changes at this point.

coriolinus
coriolinus requested changes Sep 18, 2023
View reviewed changes
src/impls/from_iterator.rs Outdated Show resolved Hide resolved
src/impls/create.rs Outdated Show resolved Hide resolved
src/impls/create.rs Show resolved Hide resolved
@chris-ha458
Copy link
Contributor Author

It seems that the build is failing on many of the test that use init
I addressed what you mentioned in the comments.
Would you also prefer that I modify the FromIterator impls from_iter fns correspondingly?

@coriolinus
Copy link
Owner

Build was not failing but warning on the tests. But good point. #42 fixes those. I've merged latest master to incorporate those changes.

Yes, if you're willing, please do update the from_iter impls to take advantage of size hinting where available.

@coriolinus coriolinus self-requested a review September 18, 2023 15:17
@chris-ha458
Copy link
Contributor Author

Sure. I am willing but it might take more time.
If there are any blockers to this PR as is, please let me know so that any delay in implementing from_iter impls does not leave this in an unmergable state.

coriolinus
coriolinus reviewed Sep 19, 2023
View reviewed changes
src/impls/from_iterator.rs Outdated Show resolved Hide resolved
src/unit_tests.rs Outdated Show resolved Hide resolved
chris-ha458 and others added 4 commits September 19, 2023 17:22
@chris-ha458 @coriolinus
simplify unit_test
be7a2d9 
Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
@chris-ha458
Update unit_tests.rs
6581492
@chris-ha458
omit init_with_capacity
f5cc079
@chris-ha458
cargo fmt
8c2aeeb
coriolinus
coriolinus approved these changes Sep 19, 2023
View reviewed changes
Copy link
Owner

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! If you want to merge now and move the size_hint optimistic guessing into a new PR, that's fine. Otherwise, I'll reapprove once that's ready.

@chris-ha458
Copy link
Contributor Author

I'm on it right now, and I have a feeling we might need to repeat several rounds of back and forth regarding code style and specific methods to implement it, so I think a separate PR would be more concise.

If you merge this I'll do a #![allow(clippy::must_use_candidate)] PR and then the size hint PR

@coriolinus coriolinus merged commit fee2fc1 into coriolinus:master Sep 19, 2023
1 check passed
@chris-ha458 chris-ha458 deleted the with_capacity branch September 19, 2023 15:13
ruancomelli referenced this pull request in ruancomelli/learning-rust Jul 1, 2024
@renovate
fix(deps): update rust crate counter to 0.6.0 (#37)
64a7067 
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [counter](https://togithub.com/coriolinus/counter-rs) | dependencies |
minor | `0.5.7` -> `0.6.0` |

---

### Release Notes

<details>
<summary>coriolinus/counter-rs (counter)</summary>

###
[`v0.6.0`](https://togithub.com/coriolinus/counter-rs/releases/tag/v0.6.0)

[Compare
Source](https://togithub.com/coriolinus/counter-rs/compare/v0.5.7...v0.6.0)

#### What's Changed

- update edition, add `counter` keyword by
[@&#8203;chris-ha458](https://togithub.com/chris-ha458) in
[https://github.com/coriolinus/counter-rs/pull/34](https://togithub.com/coriolinus/counter-rs/pull/34)
- refactor tests and impls into distinct modules by
[@&#8203;chris-ha458](https://togithub.com/chris-ha458) in
[https://github.com/coriolinus/counter-rs/pull/36](https://togithub.com/coriolinus/counter-rs/pull/36)
- small doc formatting by
[@&#8203;chris-ha458](https://togithub.com/chris-ha458) in
[https://github.com/coriolinus/counter-rs/pull/38](https://togithub.com/coriolinus/counter-rs/pull/38)
- deprecate `init` method by
[@&#8203;coriolinus](https://togithub.com/coriolinus) in
[https://github.com/coriolinus/counter-rs/pull/41](https://togithub.com/coriolinus/counter-rs/pull/41)
- do not use deprecated `init` method by
[@&#8203;coriolinus](https://togithub.com/coriolinus) in
[https://github.com/coriolinus/counter-rs/pull/42](https://togithub.com/coriolinus/counter-rs/pull/42)
- With capacity by
[@&#8203;chris-ha458](https://togithub.com/chris-ha458) in
[https://github.com/coriolinus/counter-rs/pull/40](https://togithub.com/coriolinus/counter-rs/pull/40)
- Clippy fixes by
[@&#8203;chris-ha458](https://togithub.com/chris-ha458) in
[https://github.com/coriolinus/counter-rs/pull/43](https://togithub.com/coriolinus/counter-rs/pull/43)
- Implement Serialize and Deserialize by
[@&#8203;matthewmcintire-savantx](https://togithub.com/matthewmcintire-savantx)
in
[https://github.com/coriolinus/counter-rs/pull/46](https://togithub.com/coriolinus/counter-rs/pull/46)

#### New Contributors

- [@&#8203;chris-ha458](https://togithub.com/chris-ha458) made their
first contribution in
[https://github.com/coriolinus/counter-rs/pull/34](https://togithub.com/coriolinus/counter-rs/pull/34)
-
[@&#8203;matthewmcintire-savantx](https://togithub.com/matthewmcintire-savantx)
made their first contribution in
[https://github.com/coriolinus/counter-rs/pull/46](https://togithub.com/coriolinus/counter-rs/pull/46)

**Full Changelog**:
coriolinus/counter-rs@v0.5.7...v0.5.8

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/ruancomelli/learning-rust).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40MjEuMCIsInVwZGF0ZWRJblZlciI6IjM3LjQyMS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiLCJyZW5vdmF0ZSJdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coriolinus coriolinus coriolinus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chris-ha458 @coriolinus