ci: force no_unwrap in clippy

[Itay-Tsabary-Starkware]

This change is 

[giladchase]

Happened to notice this PR so sorry for barging in.
Note that these options are already configured here.

Reviewable status: 0 of 3 files reviewed, all discussions resolved

[giladchase]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

.clippy.toml line 2 at r1 (raw file):

allow-unwrap-in-tests = true
enum-variant-size-threshold = 500

This looks like a roundabout way of disabling clippy::enum_variant_names?
I don't think that lint should be disabled, it looks solid.

Code quote:

enum-variant-size-threshold = 500

---

scripts/clippy.sh line 3 at r1 (raw file):

#!/bin/bash

cargo clippy "$@" --all-targets --all-features -- -D warnings -D future-incompatible -D nonstandard-style -D rust-2018-idioms -D unused -D clippy::unwrap_used -A clippy::blocks_in_conditions

I suggest removing this, there are too many non-test cases where it does make sense to use unwrap --- like retrieving something from a hashmap that you just inserted into.
Grep #[allow(clippy::unwrap_used)] in papyrus to see proof of this.

Also I think papini or lior also said that unwrap is Okay occasionally, but just that PR maintainers need to be very selective about it, and only allow it when expect makes no sense.

Code quote:

 -D clippy::unwrap_used

---

scripts/clippy.sh line 3 at r1 (raw file):

#!/bin/bash

cargo clippy "$@" --all-targets --all-features -- -D warnings -D future-incompatible -D nonstandard-style -D rust-2018-idioms -D unused -D clippy::unwrap_used -A clippy::blocks_in_conditions

I think papyrus only added this due to a bug in tracing, which we don't currently use, so let's remove this plz.
(We do plan on using tracing at some point, but by then the bug fix will probably be released according to the github issue.)

Code quote:

 -A clippy::blocks_in_conditions

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @giladchase)

---

.clippy.toml line 2 at r1 (raw file):

Previously, giladchase wrote…

This looks like a roundabout way of disabling clippy::enum_variant_names?
I don't think that lint should be disabled, it looks solid.

That's the setting in the papyrus repo, I really don't mind about it at all. I just prefer to be consistent.

---

scripts/clippy.sh line 3 at r1 (raw file):

Previously, giladchase wrote…

I suggest removing this, there are too many non-test cases where it does make sense to use unwrap --- like retrieving something from a hashmap that you just inserted into.
Grep #[allow(clippy::unwrap_used)] in papyrus to see proof of this.

Also I think papini or lior also said that unwrap is Okay occasionally, but just that PR maintainers need to be very selective about it, and only allow it when expect makes no sense.

1. I moved the definitions to the Cargo.toml file.
2. I prefer to be consistent with papyrus.
3. Having this enforcement on and turning it off is much simpler than having it off and turning it on later.
4. Having default strict settings and explicitly relaxing them is preferred imo than the other way around.

---

scripts/clippy.sh line 3 at r1 (raw file):

Previously, giladchase wrote…

I think papyrus only added this due to a bug in tracing, which we don't currently use, so let's remove this plz.
(We do plan on using tracing at some point, but by then the bug fix will probably be released according to the github issue.)

Right, removed.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @giladchase)

---

.clippy.toml line 2 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

That's the setting in the papyrus repo, I really don't mind about it at all. I just prefer to be consistent.

Also, these lints address different aspects. Chat-gpt'ed:

The clippy::enum_variant_names lint is a lint provided by Clippy, the Rust linter, which checks if the names of the enum variants follow a consistent naming convention.

The clippy::enum-variant-size-threshold lint is provided by Clippy, the Rust linter. It checks whether the size of an enum variant (in terms of the number of fields it contains) exceeds a specified threshold. This lint can help you identify enum variants that are becoming overly large, which may indicate a design issue or suggest that you might want to refactor your code for clarity or efficiency.

[giladchase]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

.clippy.toml line 2 at r1 (raw file):

I just prefer to be consistent

There is also consistency with SN API and Blockifier to consider.

Chat-gpt'ed:

The official docs are better here.
Specicially, note that enum-variant-size-threshold only affects enum-variant-names lint.
So setting the former to 500 effectively means disabling the latter for all sensibly sized enums.
If you want to disable enum_variant_names, you can turn it off explicitly, but that also needs explaining, since the lint looks like it bans an relatively unreadable use of unpacking.

---

crates/gateway/src/gateway.rs line 35 at r1 (raw file):

            .serve(app.into_make_service())
            .await
            .expect("Unexpected server error.");

I think this expect doesn't add any new information, unwrap is more appropriate.
Note: the library authors themselves use unwrap for this usage, both in this link and throughout their entire codebase.

Code quote:

        axum::Server::bind(&addr)
            .serve(app.into_make_service())
            .await
            .expect("Unexpected server error.");

---

scripts/clippy.sh line 3 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

1. I moved the definitions to the Cargo.toml file.
2. I prefer to be consistent with papyrus.
3. Having this enforcement on and turning it off is much simpler than having it off and turning it on later.
4. Having default strict settings and explicitly relaxing them is preferred imo than the other way around.

1. tnx

2. What about Starknet API and blockifier, which don't have this setting?

3/4. If we follow that logic, we should enable all lints.
However, considering that the only project using this lint (papyrus) has overridden it 11 times in production code suggests it's too strict.
Plus, it promotes unnecessary expect usage, which should document assumptions, rather than boilerplate strings.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @giladchase)

---

.clippy.toml line 2 at r1 (raw file):

Previously, giladchase wrote…

I just prefer to be consistent

There is also consistency with SN API and Blockifier to consider.

Chat-gpt'ed:

The official docs are better here.
Specicially, note that enum-variant-size-threshold only affects enum-variant-names lint.
So setting the former to 500 effectively means disabling the latter for all sensibly sized enums.
If you want to disable enum_variant_names, you can turn it off explicitly, but that also needs explaining, since the lint looks like it bans an relatively unreadable use of unpacking.

Ack, removed.

---

crates/gateway/src/gateway.rs line 35 at r1 (raw file):

Previously, giladchase wrote…

I think this expect doesn't add any new information, unwrap is more appropriate.
Note: the library authors themselves use unwrap for this usage, both in this link and throughout their entire codebase.

I prefer not having unwraps; the use (or misuse) of linters in this package are orhtogonal to that.
I updated the error message according to the referenced styling guide.

---

scripts/clippy.sh line 3 at r1 (raw file):

Previously, giladchase wrote…

1. tnx

2. What about Starknet API and blockifier, which don't have this setting?

3/4. If we follow that logic, we should enable all lints.
However, considering that the only project using this lint (papyrus) has overridden it 11 times in production code suggests it's too strict.
Plus, it promotes unnecessary expect usage, which should document assumptions, rather than boilerplate strings.

My main concern is that we'll move around code across the different code bases, each with its own linter, and will face incompatibility issues. Therefore, I suggest we stick to the strictest version each of the current projects use, which seems to be the papyrus version, before we standartize this. Prehaps we can relax the linter in papyrus, I'm not arguing against that.

I'll set a meeting. In the mean time, I humbly ask we stick to the stricter version.

RE other linters: If there are other usesful ones please lmk.

RE papyrus usage and overrides: It's a huge code base, I see this as only 11 times.

Generaly, I'm strongly in favor of expect (not sure why you refer to that as unnecessary) -- following the commenting style you mentioned in a different message.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 41.17%. Comparing base (9e36a57) to head (1815a99).

Files	Patch %	Lines
crates/gateway/src/gateway.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #30   +/-   ##
=======================================
  Coverage   41.17%   41.17%           
=======================================
  Files           3        3           
  Lines          34       34           
  Branches       34       34           
=======================================
  Hits           14       14           
  Misses         20       20           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[giladchase]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/gateway/src/gateway.rs line 35 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

I prefer not having unwraps; the use (or misuse) of linters in this package are orhtogonal to that.
I updated the error message according to the referenced styling guide.

"Server should run forever" is not the assumption, it is the requirement.

If you don't want to follow the docs, then you'll have to go into serve and see why this invocation will never fail.
This is probably not the way to go, as this will soon be replaced by a central error handling enum.
If you insist on banning unwrap, you'll have to add clippy ignore here.

---

scripts/clippy.sh line 3 at r1 (raw file):

My main concern is that we'll move around code across the different code bases, each with its own linter, and will face incompatibility issues. Therefore, I suggest we stick to the strictest version each of the current projects use, which seems to be the papyrus version, before we standartize this. Prehaps we can relax the linter in papyrus, I'm not arguing against that.

Do you have a use case in mind in which we move code from the mempool to papyrus?

RE other linters: If there are other usesful ones please lmk.

There's a disconnect. I claim that by your points 3 and 4, all linters should be on by default, because adding them later will be hard.
The reason why some of them are of is because they were deemed pedantic by the writers, or are too obtrusive.
For example, if you ban unwrap, then even if you're playing around locally while developing, you won't be able to compile any code the unwraps; you'll have to add a bunch of .expect("ASDF") things, just to get things to compile in order to check something.

RE papyrus usage and overrides: It's a huge code base, I see this as only 11 times.

There is also incorrect usage of expect, which we cannot grep for easily.

Generaly, I'm strongly in favor of expect (not sure why you refer to that as unnecessary) -- following the commenting style you mentioned in a different message.

I didn't say that all expect is unnecessary, I said that banning unwraps promotes unnecessary usage of expects, which is what that link hinted at: if you don't have anything meaningful to say in an expect, you should not use expect. This is a very rare occurrence, therefore valid unwraps are relatively rare.

.

[Itay-Tsabary-Starkware]

Closing pull request: commit has gone away