- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 76
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
[New Output] Support Note Output #311
[New Output] Support Note Output #311
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #311 +/- ##
==========================================
+ Coverage 90.16% 90.20% +0.04%
==========================================
Files 17 17
Lines 2470 2481 +11
==========================================
+ Hits 2227 2238 +11
Misses 243 243 ☔ View full report in Codecov by Sentry. |
@cpisciotta Currently, the tests do not check if the strings are coloured correctly if I am not mistaken. Do you think this should be covered? What do you think about the formatting I chose for this output? |
…erers. Additionally, I had to decrease the magic numbers in ParsingTests.swift.
62a9ad6
to
37cb126
Compare
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.
Hey @ikelax! I appreciate your contribution. Happy to get this merged in, but can you fix the conflicts first? Thanks!
/// Regular expression captured groups: | ||
/// $1 = note | ||
/// $2 = information | ||
static let regex = Regex(pattern: "^(note:) (.*)") |
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.
nit: We can change this to `"^note: (.*)$".
No need to capture note
, since it's a static keyword.
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 fix it.
I think it LGTM! No need to test the colored output. |
In the last commit, I removed the capturing of "note: " because it is a static keyword and forgot to adapt the comment. Only the actual note is captured.
@cpisciotta Thank you for review! Sorry, for the delay. I fixed the merge conflicts, added a test for the Azure DevOps pipeline renderer and made the change suggested by you. |
func testNotMatchNote() throws { | ||
let inputs = [ | ||
"in the note middle", | ||
"in the note: middle", | ||
"note Building targets in dependency order", | ||
"Note Metadata extraction skipped.", | ||
"Target dependency graph (12 targets) note", | ||
"Target dependency graph (12 targets) note:", | ||
"Target dependency graph (12 targets): note:", | ||
] | ||
|
||
for input in inputs { | ||
XCTAssertNil(parser.parse(line: input)) | ||
} | ||
} |
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.
Nice. Thanks for adding this test.
Thanks for your contribution @ikelax! |
Adresses #208.
Changes
Add a capture group for the Note output.
Testing