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

refactor: initialised clippy::missing_panics_doc #188

Merged
merged 21 commits into from
Oct 5, 2024

Conversation

Gmin2
Copy link
Contributor

@Gmin2 Gmin2 commented Sep 26, 2024

Rationale for this change

In order to improve documentation and identify possible panics, we should enable clippy lints that warn when a function that panics but does not document this behavior.

What changes are included in this PR?

Added panic doc comments for all the unwrap() statements

Fixes #163
/claim #163

@Gmin2
Copy link
Contributor Author

Gmin2 commented Sep 26, 2024

@JayWhite2357 , to complete the doc part of it will take two to three days

Copy link
Contributor

@JayWhite2357 JayWhite2357 left a comment

Choose a reason for hiding this comment

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

If you get this to pass CI, I'm happy to merge this PR while you work on the docs in a separate PR.
It would prevent any new code from being undocumented in the meantime.

Cargo.toml Outdated Show resolved Hide resolved
clippy.toml Outdated Show resolved Hide resolved
crates/proof-of-sql-parser/src/identifier.rs Outdated Show resolved Hide resolved
crates/proof-of-sql-parser/src/intermediate_ast.rs Outdated Show resolved Hide resolved
@JayWhite2357
Copy link
Contributor

Since you've contributed a couple PRs now, it would be helpful for you to be a bit more verbose on the merge requests.
Check out some of the recently merged PRs for some examples: #181, #180.

@JayWhite2357
Copy link
Contributor

@Gmin2 it looks like I can't approve the CI workflows unless the branch conflicts are resolved.

@Gmin2
Copy link
Contributor Author

Gmin2 commented Sep 27, 2024

Hey @JayWhite2357, i think it will make sense to include docs in this PR only, wdyt?

@JayWhite2357
Copy link
Contributor

Hey @JayWhite2357, i think it will make sense to include docs in this PR only, wdyt?

It doesn't matter to me. Entirely up to you.

@Gmin2 Gmin2 changed the title WIP!: initialised clippy::missing_panics_doc refactor!: initialised clippy::missing_panics_doc Sep 29, 2024
@Gmin2 Gmin2 requested a review from JayWhite2357 September 29, 2024 09:33
@Gmin2
Copy link
Contributor Author

Gmin2 commented Sep 29, 2024

Hey @JayWhite2357, completed all the panic docs for unwrap() statements, but the assert statements are giving warnings what should i do about them ?

@JayWhite2357
Copy link
Contributor

Hey @JayWhite2357, completed all the panic docs for unwrap() statements, but the assert statements are giving warnings what should i do about them ?

@Gmin2

If the assert is guaranteed to not panic, then it is safe to use allow(clippy::missing_panics_doc, reason = "some reasoning why we should not include a panic doc").
Be sure that the reason is thorough. If needed, put a short reason in the reason = "..." and if a more verbose explanation is needed, put more in some comments (not docs).

Copy link
Contributor

@JayWhite2357 JayWhite2357 left a comment

Choose a reason for hiding this comment

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

I started reviewing some of this.

The general feedback I have on this: there are some cases where an unwrap or another panic could occur, but based on the logic of the methods, doesn't occur. This should not be included in the Panic documentation, and instead should be expected away and explained in comments (no docs).

Additionally, there are a couple of spots I noted where the panic documentation could be a bit more descriptive on the root cause of why the panic occurs.

In both cases, you may have to look at, for example, why a method returns None so that you can explain either why a panic might occur or why it cannot occur.

crates/proof-of-sql-parser/src/identifier.rs Outdated Show resolved Hide resolved
crates/proof-of-sql-parser/src/utility.rs Outdated Show resolved Hide resolved
crates/proof-of-sql-parser/src/utility.rs Outdated Show resolved Hide resolved
crates/proof-of-sql/src/base/polynomial/interpolate.rs Outdated Show resolved Hide resolved
crates/proof-of-sql/src/base/polynomial/interpolate.rs Outdated Show resolved Hide resolved
crates/proof-of-sql/src/base/polynomial/interpolate.rs Outdated Show resolved Hide resolved
crates/proof-of-sql/src/base/polynomial/interpolate.rs Outdated Show resolved Hide resolved
crates/proof-of-sql/src/base/scalar/mont_scalar.rs Outdated Show resolved Hide resolved
crates/proof-of-sql/src/base/scalar/mont_scalar.rs Outdated Show resolved Hide resolved
@JayWhite2357
Copy link
Contributor

@Gmin2 thanks for the effort here. It is looking good!

@JayWhite2357 JayWhite2357 self-requested a review October 2, 2024 01:47
@Gmin2
Copy link
Contributor Author

Gmin2 commented Oct 4, 2024

@Gmin2 thanks for the effort here. It is looking good!

Hey @JayWhite2357 , i have added all the changes you have mentioned, can you take a look and one more thing can we wrap this quickly because the merge conflicts are getting bigger and bigger, if somethings needs to be adjusted we can do it in the followup PR

@JayWhite2357 JayWhite2357 enabled auto-merge (squash) October 5, 2024 02:44
@JayWhite2357 JayWhite2357 disabled auto-merge October 5, 2024 02:45
@JayWhite2357 JayWhite2357 changed the title refactor!: initialised clippy::missing_panics_doc refactor: initialised clippy::missing_panics_doc Oct 5, 2024
@JayWhite2357 JayWhite2357 enabled auto-merge (squash) October 5, 2024 02:46
@JayWhite2357 JayWhite2357 merged commit b70c212 into spaceandtimelabs:main Oct 5, 2024
11 checks passed
@JayWhite2357
Copy link
Contributor

/approve

Copy link

algora-pbc bot commented Oct 5, 2024

@JayWhite2357: The claim has been successfully added to reward-all. You can visit your dashboard to complete the payment.

Copy link

github-actions bot commented Oct 5, 2024

🎉 This PR is included in version 0.27.7 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable clippy::missing_panics_doc
2 participants