Skip to content

A lot of improvements in Set, mostly docs and doc-tests code. #284

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

Merged
merged 16 commits into from
Apr 22, 2025

Conversation

owtotwo
Copy link
Contributor

@owtotwo owtotwo commented Apr 21, 2025

First of all, please don't be surprised by the large amount of changes.

There are so many code changes because I should have divided them into several or even more small PRs, but this would make the merging process take a lot of time for you (even though most of them have no intersection).

In fact, these changes themselves are not complicated. You can clearly see them by looking at the changes and the corresponding commit messages one by one in chronological order.

Additional note: There is still a lot of discussion on the Internet about #[must_use].
Check this: https://rust-lang.github.io/rust-clippy/master/index.html#/must_use_candidate

My opinion is that the Clippy should let the pure functions be set #[must_use] by default, and functions with side effects be explicitly marked.
But Rust offical linter (clippy) have apparently not yet considered the relevant issues, so we won't use it for now (otherwise it would probably fill up the world).

This PR has largely improved the content of the project documentation and document testing to a "sufficient" level.
(And it takes some time, I can even use it to complete a game of Civilization 6. 😆)

@owtotwo
Copy link
Contributor Author

owtotwo commented Apr 21, 2025

@yegor256 I hope the commit message contains enough information! (If not so, I can explain what you are confused about.)

@owtotwo
Copy link
Contributor Author

owtotwo commented Apr 22, 2025

@yegor256 Is there anything I need to modify?

@yegor256
Copy link
Owner

@owtotwo in general, I believe that having more restrictions is better than having less of them. If we remove the #[must_use], we make it possible for programmers to ignore the results returned by the function, right? What do we gain with this? I know what we lose: strictness.

@owtotwo
Copy link
Contributor Author

owtotwo commented Apr 22, 2025

@yegor256 Where do you think #[must_use] should be added?

@owtotwo
Copy link
Contributor Author

owtotwo commented Apr 22, 2025

@yegor256 To be clear, I did not give up on #[must_use]. I just removed the mark added by the wrong lint in this PR(such as remove() or remove_entry()), and waited for us to confirm where to add #[must_use] before we opened a new PR to do this.

This was a very influential decision.
I have experienced the version where #[must_use] was stabilized, and I have also followed(many years ago) the discussion of adding a large number of #[must_use] to the standard library and a large number of modification PRs to the standard library.

I know that the warnings issued by #[must_use] play a significant role in discovering potential bugs.

I will definitely not give up the benefits it brings us, because as I said in the description above, we will "temporarily" remove them ("for now"), then discuss where to add them, and finally release a large number of changes (this is a breaking change for #![deny(warnings)])

Therefore, before we make a large number of modifications, we remove it first (Actually, the main purpose is to remove the wrong mark caused by wrong lints), and then determine the corresponding addition rules and release a special PR.

@yegor256
Copy link
Owner

@rultor merge

@yegor256
Copy link
Owner

@owtotwo make sense, thanks

@rultor
Copy link
Collaborator

rultor commented Apr 22, 2025

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here.

@rultor rultor merged commit 8a8e65d into yegor256:master Apr 22, 2025
14 checks passed
@rultor
Copy link
Collaborator

rultor commented Apr 22, 2025

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 9min).

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.

3 participants