-
Notifications
You must be signed in to change notification settings - Fork 362
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
Spelling #1101
Spelling #1101
Conversation
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1101 +/- ##
===========================================
- Coverage 100.00% 99.98% -0.02%
===========================================
Files 17 17
Lines 4546 5019 +473
Branches 0 1026 +1026
===========================================
+ Hits 4546 5018 +472
- Misses 0 1 +1 ☔ View full report in Codecov by Sentry. |
@@ -514,7 +514,7 @@ On the command line, options can be given as: | |||
- `-ffilename` (no space required) | |||
- `-abcf filename` (flags and option can be combined) | |||
- `--long` (long flag) | |||
- `--long_flag=true` (long flag with equals to override default value) | |||
- `--long_flag=true` (long flag with equals -- to override default value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a little different from most changes and is very manual (all changes are manual, but this is more so). I'm happy to drop it or anything else, but I think it kinda helps to clarify that the second part describes the first.
@@ -71,7 +71,7 @@ | |||
# - Added the option for users to set the GCOVR_ADDITIONAL_ARGS variable to supply additional | |||
# flags to the gcovr command | |||
# | |||
# 2020-05-04, Mihchael Davis | |||
# 2020-05-04, Michael Davis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fairly certain this is the correct person's name, but I'm not really sure where this code comes from -- I'd love to fix the source material.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original is here https://github.com/bilke/cmake-modules/blob/master/CodeCoverage.cmake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I posted https://github.com/bilke/cmake-modules/pull/90/files#r1878500171 to upstream this fix.
@@ -163,7 +163,7 @@ struct pair_adaptor< | |||
} | |||
}; | |||
|
|||
// Warning is suppressed due to "bug" in gcc<5.0 and gcc 7.0 with c++17 enabled that generates a Wnarrowing warning | |||
// Warning is suppressed due to "bug" in gcc<5.0 and gcc 7.0 with c++17 enabled that generates a -Wnarrowing warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding the -
isn't unreasonable...
#if defined CLI11_HAS_FILEYSTEM && CLI11_HAS_FILESYSTEM > 0 && defined(_MSC_VER) | ||
#if defined CLI11_HAS_FILESYSTEM && CLI11_HAS_FILESYSTEM > 0 && defined(_MSC_VER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notably, this appears to be a broken guard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is probably triggering the coverage issue, meaning some bits of code were never run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the extra coverage in https://app.codecov.io/gh/CLIUtils/CLI11/pull/1101/blob/include/CLI/TypeTools.hpp definitely would make sense based on being able to run this additional line:
Line 1313 in bdd12bf
auto fspclass = CLI::detail::classify_object<std::filesystem::path>::value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will see about getting the coverage back in a different PR
@@ -131,7 +131,7 @@ add_test(NAME shapes_all COMMAND shapes circle 4.4 circle 10.7 rectangle 4 4 cir | |||
4.5 ++ rectangle 2.1 ++ circle 234.675) | |||
set_property( | |||
TEST shapes_all PROPERTY PASS_REGULAR_EXPRESSION "circle2" "circle4" | |||
"rectangle2 with edges [2.1,2.1]" "triangel1 with sides [4.5]") | |||
"rectangle2 with edges [2.1,2.1]" "triangle1 with sides [4.5]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have wondered that before but never looking into it, I don't think these tests are doing what we think they are doing. This should have been a test failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obviously not an issue to fix in this PR
Is that action generating the commit(s) automatically? Is it something that could be run regularly on a CI job? |
The action only complains about spelling. Commits require a human. There are some tools that automatically suggest corrections (I'm not particularly enamored by any of them). I have a spreadsheet that I've used in the past to get Google to make suggestions which can generate commands that I use with another script to make uniform commits. Recently I've just written the commands out by hand since I haven't had enough corrections relative to unknown words to justify using the spreadsheet. |
This check seems to be catching more than was caught with the codespell run as part of pre-commit. Wondering if it is something we should be running regularly? |
That's my general experience -- I regularly find bugs in repositories that use codespell. It's easy enough to configure. If you're interested, I can make a PR to enable it after this is merged. There are a couple of tunables worth discussing. (The sample workflow here is using a prerelease version, so I'd switch it to the current shipping release...) |
@jsoref I will probably dig deeper in the spelling for another repo, but I am going to merge this one. Thank You! |
@phlptp fwiw, I've rebased my checks and converted them from my prerelease to the latest release, (there's one spelling error that appears to have snuck in between when I prepared this PR and when it was merged), if you're interested in playing with the code, it's available here: https://github.com/jsoref/CLI11/tree/refs/heads/spell-check-with-spelling. Or, if you want to play with check-spelling, you can try dropping https://github.com/check-spelling/spell-check-this/blob/prerelease/.github/workflows/spelling.yml into the workflows directory of another repository and let check-spelling walk you through adapting check-spelling to a repository. |
This PR corrects misspellings identified by the check-spelling action (which is an evolution of the script I used ages ago when I first made a PR here...).
The misspellings have been reported at https://github.com/jsoref/CLI11/actions/runs/12194174338#summary-34017587518
The action reports that the changes in this PR would make it happy: https://github.com/jsoref/CLI11/actions/runs/12194174680#summary-34017588281