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

Extend --list command output to show which comparators do not run by default #55

Merged

Conversation

marciniwanicki
Copy link
Contributor

Resolves #49

Testing performed

  • CI
  • Run xcdiff --list, ensure all except RESOLVED_SETTINGS comparators are listed with default value Yes.

@codecov
Copy link

codecov bot commented May 23, 2020

Codecov Report

Merging #55 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
+ Coverage   95.77%   95.82%   +0.05%     
==========================================
  Files          43       43              
  Lines        1467     1486      +19     
==========================================
+ Hits         1405     1424      +19     
  Misses         62       62              
Impacted Files Coverage Δ
Sources/XCDiffCommand/CommandRunner.swift 95.45% <100.00%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50c0cb4...a7ad82c. Read the comment docs.

kwridan
kwridan previously approved these changes May 26, 2020
Copy link
Contributor

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

👍 left some minor wording suggestions

- RESOLVED_SETTINGS
- SOURCE_TREES
- LINKED_DEPENDENCIES
COMPARTOR TAG | DEFAULT
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the wording needs a slight tweak or possibly additional text is needed?

e.g. It may not be obvious what Default means in this context?

Some ideas:

  COMPARTOR TAG                | INCLUDED BY DEFAULT  
------------------------------------------------------
- FILE_REFERENCES              | Yes
- BUILD_PHASES                 | Yes
- COPY_FILES                   | Yes
- TARGETS                      | Yes
- HEADERS                      | Yes
- SOURCES                      | Yes
- RESOURCES                    | Yes
- CONFIGURATIONS               | Yes
- SETTINGS                     | Yes
- RESOLVED_SETTINGS            | No
- SOURCE_TREES                 | Yes
- LINKED_DEPENDENCIES          | Yes

It's a bit too long

The following list shows all available comparator tags along with their default inclusion status when tags aren't explicitly specified.

  COMPARTOR TAG                | INCLUDED 
------------------------------------------------------
- FILE_REFERENCES              | Yes
- BUILD_PHASES                 | Yes
- COPY_FILES                   | Yes
- TARGETS                      | Yes
- HEADERS                      | Yes
- SOURCES                      | Yes
- RESOURCES                    | Yes
- CONFIGURATIONS               | Yes
- SETTINGS                     | Yes
- RESOLVED_SETTINGS            | No
- SOURCE_TREES                 | Yes
- LINKED_DEPENDENCIES          | Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @kwridan, good idea. I went for the 2nd option

kwridan
kwridan previously approved these changes May 29, 2020
… default

Signed-off-by: Marcin Iwanicki <miwanicki1@bloomberg.net>
Signed-off-by: Marcin Iwanicki <miwanicki1@bloomberg.net>
@marciniwanicki
Copy link
Contributor Author

👋 @kwridan, I had to rebase, we will need one more re-✅.

@marciniwanicki
Copy link
Contributor Author

Thank you : -)

@marciniwanicki marciniwanicki merged commit 6ceb728 into bloomberg:master May 30, 2020
@marciniwanicki marciniwanicki deleted the feature/49-list-with-defaults branch May 30, 2020 12:30
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.

xcdiff --list could show which tags are enabled/disabled by default
2 participants