Downgrade option_option to pedantic #5401
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Some discussion about this lint was already done in #97. @Manishearth made the argument, that a custom tri-state enum would be better than an r? @Manishearth |
|
@bors r+ |
|
📌 Commit f6e8da8 has been approved by |
bors
added a commit
that referenced
this pull request
Apr 1, 2020
Downgrade option_option to pedantic
Based on a search of my work codebase (\>500k lines) for `Option<Option<`, it looks like a bunch of reasonable uses to me. The documented motivation for this lint is:
> an optional optional value is logically the same thing as an optional value but has an unneeded extra level of wrapping
which seems a bit bogus in practice. For example a typical usage would look like:
```rust
let mut host: Option<String> = None;
let mut port: Option<i32> = None;
let mut payload: Option<Option<String>> = None;
for each field {
match field.name {
"host" => host = Some(...),
"port" => port = Some(...),
"payload" => payload = Some(...), // can be null or string
_ => return error,
}
}
let host = host.ok_or(...)?;
let port = port.ok_or(...)?;
let payload = payload.ok_or(...)?;
do_thing(host, port, payload)
```
This lint seems to fit right in with the pedantic group; I don't think linting on occurrences of `Option<Option<T>>` by default is justified.
---
changelog: Remove option_option from default set of enabled lints
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit f6e8da8 has been approved by |
bors
added a commit
that referenced
this pull request
Apr 1, 2020
Downgrade option_option to pedantic
Based on a search of my work codebase (\>500k lines) for `Option<Option<`, it looks like a bunch of reasonable uses to me. The documented motivation for this lint is:
> an optional optional value is logically the same thing as an optional value but has an unneeded extra level of wrapping
which seems a bit bogus in practice. For example a typical usage would look like:
```rust
let mut host: Option<String> = None;
let mut port: Option<i32> = None;
let mut payload: Option<Option<String>> = None;
for each field {
match field.name {
"host" => host = Some(...),
"port" => port = Some(...),
"payload" => payload = Some(...), // can be null or string
_ => return error,
}
}
let host = host.ok_or(...)?;
let port = port.ok_or(...)?;
let payload = payload.ok_or(...)?;
do_thing(host, port, payload)
```
This lint seems to fit right in with the pedantic group; I don't think linting on occurrences of `Option<Option<T>>` by default is justified.
---
changelog: Remove option_option from default set of enabled lints
|
💔 Test failed - checks-action_test |
|
Looks like an unrelated CI failure. 2020-04-01T20:49:41.7428390Z Err:53 http://azure.archive.ubuntu.com/ubuntu bionic InRelease
2020-04-01T20:49:41.7428704Z Connection failed [IP: 52.177.174.250 80]
2020-04-01T20:50:12.0005500Z Err:54 http://azure.archive.ubuntu.com/ubuntu bionic-updates InRelease
2020-04-01T20:50:12.0011660Z Could not connect to azure.archive.ubuntu.com:80 (52.177.174.250), connection timed out [IP: 52.177.174.250 80]
2020-04-01T20:50:12.0017303Z Err:55 http://azure.archive.ubuntu.com/ubuntu bionic-backports InRelease
2020-04-01T20:50:12.0017817Z Unable to connect to azure.archive.ubuntu.com:http: [IP: 52.177.174.250 80]
2020-04-01T20:50:15.8242272Z Fetched 3623 kB in 1min 33s (38.8 kB/s)
2020-04-01T20:50:16.8473136Z Reading package lists...
2020-04-01T20:50:16.8784819Z W: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/dists/bionic/InRelease Connection failed [IP: 52.177.174.250 80]
2020-04-01T20:50:16.8786623Z W: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/dists/bionic-updates/InRelease Could not connect to azure.archive.ubuntu.com:80 (52.177.174.250), connection timed out [IP: 52.177.174.250 80]
2020-04-01T20:50:16.8787633Z W: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/dists/bionic-backports/InRelease Unable to connect to azure.archive.ubuntu.com:http: [IP: 52.177.174.250 80]
2020-04-01T20:50:16.8788232Z W: Some index files failed to download. They have been ignored, or old ones used instead.
2020-04-01T20:50:16.9652733Z Reading package lists...
2020-04-01T20:50:17.1264284Z Building dependency tree...
2020-04-01T20:50:17.1276061Z Reading state information...
2020-04-01T20:50:17.1866118Z Some packages could not be installed. This may mean that you have
2020-04-01T20:50:17.1866832Z requested an impossible situation or if you are using the unstable
2020-04-01T20:50:17.1867264Z distribution that some required packages have not yet been created
2020-04-01T20:50:17.1867869Z or been moved out of Incoming.
2020-04-01T20:50:17.1868325Z The following information may help to resolve the situation:
2020-04-01T20:50:17.1868600Z
2020-04-01T20:50:17.1868901Z The following packages have unmet dependencies:
2020-04-01T20:50:17.2470010Z libgit2-dev:i386 : Depends: libgit2-26:i386 (= 0.26.0+dfsg.1-1.1ubuntu0.2) but it is not going to be installed
2020-04-01T20:50:17.2470799Z Depends: libz-dev:i386 but it is not installable
2020-04-01T20:50:17.2471164Z Depends: libcurl4-gnutls-dev:i386 but it is not going to be installed
2020-04-01T20:50:17.2471510Z Depends: libssh2-1-dev:i386 but it is not installable
2020-04-01T20:50:17.2471880Z Depends: libhttp-parser-dev:i386 but it is not installable
2020-04-01T20:50:17.2474916Z libssl-dev:i386 : Depends: libssl1.1:i386 (= 1.1.1d-1+ubuntu18.04.1+deb.sury.org+2) but it is not going to be installed
2020-04-01T20:50:17.2834733Z E: Unable to correct problems, you have held broken packages.
2020-04-01T20:50:17.2856894Z ##[error]Process completed with exit code 100. |
|
@bors retry |
1 similar comment
|
@bors retry |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on a search of my work codebase (>500k lines) for
Option<Option<, it looks like a bunch of reasonable uses to me. The documented motivation for this lint is:which seems a bit bogus in practice. For example a typical usage would look like:
This lint seems to fit right in with the pedantic group; I don't think linting on occurrences of
Option<Option<T>>by default is justified.changelog: Remove option_option from default set of enabled lints