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

Compare command chores #7947

Merged
merged 5 commits into from
Nov 28, 2023
Merged

Compare command chores #7947

merged 5 commits into from
Nov 28, 2023

Conversation

sschuberth
Copy link
Member

Please have a look at the individual commit messages for the details.

While the `enum class` does not need command context, a nested class in
Kotlin does not have outer class context anyway (only `inner` classes
have), so it does not hurt to scope the enum to the command where it is
being used.

Signed-off-by: Sebastian Schuberth <[email protected]>
Use a more general name as this is now used for serializing, too.

Signed-off-by: Sebastian Schuberth <[email protected]>
Use code 1 for fundamental errors that prevent the command from properly
executing, and codes >= 2 to signal failures detected as part of running
the command properly.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth requested a review from a team as a code owner November 28, 2023 13:48
@sschuberth sschuberth enabled auto-merge (rebase) November 28, 2023 13:48
Copy link

codecov bot commented Nov 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f1abea1) 66.99% compared to head (b068067) 66.99%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7947   +/-   ##
=========================================
  Coverage     66.99%   66.99%           
  Complexity     2041     2041           
=========================================
  Files           356      356           
  Lines         17103    17103           
  Branches       2443     2443           
=========================================
  Hits          11459    11459           
  Misses         4623     4623           
  Partials       1021     1021           
Flag Coverage Δ
funTest-non-docker 34.35% <ø> (ø)
test 36.19% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@@ -70,6 +71,12 @@ class CompareCommand : OrtCommand(
).enum<CompareMethod>()
.default(CompareMethod.TEXT_DIFF)

private val contextSize by option(
"--context-size", "-s",
help = "The context size to use for the unified diff."
Copy link
Member

Choose a reason for hiding this comment

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

Could we make the help explain a bit what the context size actually is?

e.g. "number of lines to be copied" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, this is not the "number of lines to be copied", but the number of (unmodified) lines displayed in context of a modified like, just like for the diff or git diff commands. I'll try to find a compact wording.

@@ -70,6 +71,12 @@ class CompareCommand : OrtCommand(
).enum<CompareMethod>()
.default(CompareMethod.TEXT_DIFF)

private val contextSize by option(
"--context-size", "-s",
Copy link
Member

Choose a reason for hiding this comment

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

How about using -c instead of -s like grep and diff ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The mentioned tools use upper-case -C, however. I'm now using that as well.

Copy link
Member

Choose a reason for hiding this comment

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

They use both

diff --help | grep context
  -c, -C NUM, --context[=NUM]   output NUM (default 3) lines of copied context

Copy link
Member Author

@sschuberth sschuberth Nov 28, 2023

Choose a reason for hiding this comment

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

Not on my Linux box:

$ grep --help | grep -- "-[cC],"
  -c, --count               print only a count of selected lines per FILE
  -C, --context=NUM         print NUM lines of output context
$ grep --version
grep (GNU grep) 3.11
$ git grep --help | grep -- "-[cC][, ]"
                  [ -o | --only-matching ] [-c | --count] [--all-match] [-q | --quiet]
                  [-A <post-context>] [-B <pre-context>] [-C <context>]
       -c, --count
       -<num>, -C <num>, --context <num>

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, sorry, you were looking at diff and me at grep... I guess diff makes more sense as a reference, but since -C seem to be the "lowest common denominator", shall we stick to it?

@sschuberth sschuberth requested review from fviernau and a team November 28, 2023 17:23
@sschuberth sschuberth merged commit d591aec into main Nov 28, 2023
22 checks passed
@sschuberth sschuberth deleted the compare-cmd-chores branch November 28, 2023 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants