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 flush api to default stdout/stderr logger #87

Merged
merged 2 commits into from
Aug 13, 2019
Merged

Conversation

tomerd
Copy link
Member

@tomerd tomerd commented Aug 9, 2019

motivation: default logger may not flush the streams on crush, so need to make flushing moe explicit

changes:

  • add a constructor argument to control flushing behaviour, default to "everytime"
  • add a "flush" method to allow user defined flush timing. this api is now internal but we could expose it if there is a need
  • add tests

motivation: default logger may not flush the streams on crush, so need to make flushing moe explicit

changes:
* add a constructor argument to control flushing behaviour, default to "everytime"
* add a "flush" method to allow user defined flush timing. this api is now internal but we could expose it if there is a need
* add tests
@tomerd tomerd requested a review from weissi August 9, 2019 16:12
@tomerd
Copy link
Member Author

tomerd commented Aug 9, 2019

solved #86

@tomerd tomerd added the 🔨 semver/patch No public API change. label Aug 9, 2019
var fds: [Int32] = [-1, -1]
fds.withUnsafeMutableBufferPointer { ptr in
let err = pipe(ptr.baseAddress!)
assert(err == 0, "pipe: \(err)")
Copy link
Member

Choose a reason for hiding this comment

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

because it's in a test case, you can use XCTAssertEqual(0, err)

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

Thanks! Looks great, some minor comments.

Tests/LoggingTests/LoggingTest.swift Outdated Show resolved Hide resolved
Tests/LoggingTests/LoggingTest.swift Outdated Show resolved Hide resolved
Tests/LoggingTests/LoggingTest.swift Outdated Show resolved Hide resolved
@tomerd
Copy link
Member Author

tomerd commented Aug 12, 2019

@weissi addressed

Copy link
Member

@weissi weissi 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! LGTM

@weissi weissi merged commit e309184 into apple:master Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants