filter::parse_spec() ignore bogon empty, blank (sub)strings #188
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
salewski
force-pushed
the
ads/parse-spec-ignore-blank-spec
branch
from
954267a
to
96e3e84
Compare
|
Just a heads-up that the "Check formatting" CI test is flagging an issue unrelated to this changeset[0]. I would fix that up in a separate PR, but since it wants to remove a trailing comma I'm not even sure if that change is wanted. Leaving the trailing commas seems to be more idiomatic, so maybe the lint needs tuning instead? FWIW, this is what it is saying: Run actions-rs/cargo@v1
/usr/share/rust/.cargo/bin/cargo fmt -- --check
Diff in /home/runner/work/env_logger/env_logger/src/lib.rs at line 231:
#![doc(
html_logo_url = "https://www.rust-lang.org/logos/rust-logo-128x128-blk-v2.png",
- html_favicon_url = "https://www.rust-lang.org/static/images/favicon.ico",
+ html_favicon_url = "https://www.rust-lang.org/static/images/favicon.ico"
)]
#![cfg_attr(test, deny(warnings))]
// When compiled for the rustc compiler itself we want to make sure that this is
Error: The process '/usr/share/rust/.cargo/bin/cargo' failed with exit code 1[0]: It did flag some whitespace changes in my original push, but I fixed those up and rebased. Now the above item is the only thing it is complaining about. |
These unit tests demonstrate filter::parse_spec() accepting as legit
empty and blank spec strings that it should be ignoring:
$ cargo test 'filter::tests::'
...
failures:
filter::tests::parse_spec_blank_level_isolated
filter::tests::parse_spec_blank_level_isolated_blank_comma
filter::tests::parse_spec_blank_level_isolated_comma_blank
test result: FAILED. 21 passed; 3 failed; 0 ignored; 0 measured; 11 filtered out
Fix to come in follow-up commit.
Fix issue demonstrated by unit tests in commit 2ff2bf1. While parsing comma-separated substrings, parse_spec() now trims the values to ensure empty and blank substrings (which are invalid) are ignored. Previously, only-comma-separated empty substrings were being ignored. This change extends that intent to also ignore the empty string (no comma), stand-alone blank strings (no comma), and comma- separated variations of empty and/or blank substrings.
salewski
force-pushed
the
ads/parse-spec-ignore-blank-spec
branch
from
96e3e84
to
780e7c5
Compare
|
I just rebased this and force pushed, just to pull in the |
This was referenced Mar 11, 2021
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.
This PR fixes an issue with
filter::parse_spec()accepting as legit some empty and blank spec (sub)strings that it should be ignoring.There are two commits:
The first adds five unit tests that demonstrate the issue (three of them fail).
The second commit contains the simple fix, which basically extends the intent of the existing code (ignore invalid spec strings) to cover these additional scenarios. With this change, all of the tests pass.