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.

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

Clippy fixes #43

Merged
merged 6 commits into from
Sep 19, 2023
Merged

Clippy fixes #43

merged 6 commits into from
Sep 19, 2023

Conversation

chris-ha458
Copy link
Contributor

This PR ensures that even cargo clippy -- -Wclippy::pedantic yields no warnings.

One issue : I'm not sure if k_most_common_ordered can actually panic?
It seems like logically heap won't be empty.
If it can, maybe documentation could be better so it is understood in relation to outputs.
If not, maybe we can change it to unwrap and either document that it doesn't panic or suppress the lint

@@ -38,7 +38,7 @@ where
{
fn default() -> Self {
Self {
map: Default::default(),
map: HashMap::default(),
zero: Default::default(),
Copy link
Owner

Choose a reason for hiding this comment

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

Why specify HashMap::default but not N::default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The honest answer is that because only hashmap triggered the clippy.
I think the current lint only looks for std or core traits.

Taking a closer look into the lint, it would be idiomatic to also change zero to N:: default.
added a commit for that.

@@ -499,6 +498,9 @@ where
/// be worth experimenting to see which of the two methods is faster.
///
/// [`most_common_ordered`]: Counter::most_common_ordered
///
/// # Panics
/// Panics if heap is empty
pub fn k_most_common_ordered(&self, k: usize) -> Vec<(T, N)> {
Copy link
Owner

Choose a reason for hiding this comment

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

This cannot in fact panic, because the heap is never empty. There are early returns in the function for if k == 0 or if k >= self.map.len().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed docs to reflect that it does not in fact panic as implemented

@coriolinus
Copy link
Owner

Changing an expect to an unwrap is a bad idea: if somehow the precondition does ever fail, we'll get better documentation of what we expected to hold true.

reflects the fact that, despite the use of expect, current code does not panic
src/lib.rs Outdated
Comment on lines 502 to 503
/// # Panics
/// Preconditions ensure that the current implementation does not panic.
Copy link
Owner

Choose a reason for hiding this comment

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

Is the clippy logic looking for the existence of a "Panics" section in the docs if there is a .expect in the function? I'd prefer to simply suppress that lint. Adding a big, bold-face documentation item "Panics: the current implementation does not panic" is unhelpful and pointless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppressed the lint with a comment that reflects how it doesnt panic currently.

Copy link
Owner

Choose a reason for hiding this comment

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

That suppresses the lint right now, but it introduces distracting and potentially confusing documentation. The default expectation of a function is that it does not panic, unless it has a "Panics" section in the documentation. Adding a "Panics" section which just says "this does not panic" does not help someone reading the documentation, but it has the potential to confuse them. Better to omit that section of the documentation and instead suppress the lint with an #[allow(clippy::wants_pointless_documentation)] (not the actual lint name) annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops sorry i was referring to eca86c5 I forgot to actually push before commenting.

i can also remove the insource documentation as well if you feel it is spurious

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.

This is good, thanks!

@coriolinus coriolinus merged commit 1d41465 into coriolinus:master Sep 19, 2023
1 check passed
@chris-ha458 chris-ha458 deleted the clippy_fixes branch September 19, 2023 15:13
ruancomelli referenced this pull request in ruancomelli/learning-rust Jul 1, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [counter](https://github.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://github.com/coriolinus/counter-rs/releases/tag/v0.6.0)

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

#### What's Changed

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

#### New Contributors

- [@&#8203;chris-ha458](https://github.com/chris-ha458) made their
first contribution in
[https://github.com/coriolinus/counter-rs/pull/34](https://github.com/coriolinus/counter-rs/pull/34)
-
[@&#8203;matthewmcintire-savantx](https://github.com/matthewmcintire-savantx)
made their first contribution in
[https://github.com/coriolinus/counter-rs/pull/46](https://github.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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants