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

Add 2 new properties to provide warnings and error counts based on result filter #25

Merged
merged 4 commits into from
Feb 14, 2022

Conversation

theScud
Copy link

@theScud theScud commented Nov 10, 2021

The computed properties use the same logic that is used in the report function.

Addresses Issue : #23

Note: this preliminary PR just to discuss approach i'll be adding test and docs after approach has been approved

…sult filter

The computed properties use the same logic that is used in the report function.
@@ -40,6 +40,11 @@ public final class XCodeSummary {
public var warningsCount: Int {
return warnings.count
}

/// Numbber of warnings filtered using the provided result filter

Choose a reason for hiding this comment

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

Number typo

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@@ -40,6 +40,11 @@ public final class XCodeSummary {
public var warningsCount: Int {
return warnings.count
}

/// Numbber of warnings filtered using the provided result filter
public var filteredWariningCount: Int {

Choose a reason for hiding this comment

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

filteredWariningCount typo

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@theScud
Copy link
Author

theScud commented Nov 11, 2021

@f-meloni let me if this approach works

Copy link
Owner

@f-meloni f-meloni left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR :)

@theScud
Copy link
Author

theScud commented Nov 18, 2021

@f-meloni only you have write access so i guess you'll have to merge PR.

I added test to check but i'm unable to run the test target.

Getting the following error
Screenshot 2021-11-18 at 11 47 12 AM

@f-meloni
Copy link
Owner

Hey @theScud I dropped the ball with this, sorry I completely missed your message.
This week I will try to bring back the CI to this repo, and release, but in the meantime I will merge your PR.

@f-meloni f-meloni merged commit 155cd23 into f-meloni:master Feb 14, 2022
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.

3 participants