-
Notifications
You must be signed in to change notification settings - Fork 128
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
Log Types #163
base: master
Are you sure you want to change the base?
Log Types #163
Conversation
CryptoSwift: 1.3.3 -> 1.4.2 ArgumentParser: 0.3.1 -> 1.0.3 Signed-off-by: Danny Gilbert <[email protected]>
Signed-off-by: Danny Gilbert <[email protected]>
Allow consumers to specify different logsDir Signed-off-by: Danny Gilbert <[email protected]>
Allow consumers to inject modified LogFinders Signed-off-by: Danny Gilbert <[email protected]>
Allow consumers to specify which logs they would like to parse. By default this will still apply to build logs. Allows consumers to parse things like Package resolution logs. Signed-off-by: Danny Gilbert <[email protected]>
Example: This will parse the latest package resolution log for the project. |
This reverts commit 5e36c73.
Signed-off-by: Danny Gilbert <[email protected]>
Return list of file URLs for log dir. Signed-off-by: Danny Gilbert <[email protected]>
Offer filtering for logs newer than specifed date Signed-off-by: Danny Gilbert <[email protected]>
Path is case sensitive for tests but not in real world use Signed-off-by: Danny Gilbert <[email protected]>
Signed-off-by: Danny Gilbert <[email protected]>
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.
Thanks for submitting the PR! I have a few comments and questions. I also see there are a few Swiftlint warnings
@@ -26,6 +26,9 @@ struct DumpCommand: ParsableCommand { | |||
commandName: "dump", | |||
abstract: "Dumps the xcactivitylog file into a JSON document" | |||
) | |||
|
|||
@Option(name: .long, help: "Type of .xactivitylog file to look for.") | |||
var logs: LogType = .build |
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.
Suggestion:
Rename this variable to logType
. (Same for ManifestCommand.swift and ParseCommand.swift)
@@ -26,6 +26,9 @@ struct DumpCommand: ParsableCommand { | |||
commandName: "dump", | |||
abstract: "Dumps the xcactivitylog file into a JSON document" | |||
) | |||
|
|||
@Option(name: .long, help: "Type of .xactivitylog file to look for.") |
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.
What do you think about printing the available option values here as well to make it easier for the user?
return try URL(fileURLWithPath: getLatestLogInDir(projectDir)) | ||
|
||
} | ||
|
||
public func findLatestLogsWithLogOptions(_ logOptions: LogOptions) throws -> [URL] { |
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 see this function is not used, was it supposed to be part of a refactor?
Details
Opens up the parsing feature to more LogTypes.
Idea behind this approach is to make it easier to parse things like Package logs so as to potentially allow metric collection around dependencies. For example, Package + Build Logs can provide a deeper picture of what developement cycle looks like since package resolution happen before builds take place but aren't necessarily being accounted for.