-
Notifications
You must be signed in to change notification settings - Fork 15
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
Swift rewrite #23
Swift rewrite #23
Conversation
- Moved constants and error enum inside class. - Removed prefixLogLevelForTag moving code to inside fun logLevel for optimization. - Updated tag in func logLevel to be more clear and up to date with Apple conventions.
- Adding more comments - Added new tag for Configuration.logLevel func.
…rite * 'master' of github.com:tonystone/tracelog: Ran "pod update" to update project files. Backing down deployment targets for iOS and OS X to 8.0 and 10.10 respectively. Changing iOS deployment target back to 8.0. Ran "pod update" to update project files for release. Update TraceLogTest.m: Changing #import <TraceLog/TraceLog.h> to import TraceLog; to get rid of compiler warning. Update TLAppDelegate.m: Changing #import <TraceLog/TraceLog.h> to import TraceLog;. Update Podfile: Setting new platforms for test apps. Update README.md: Create new "Requirements" section with deployment targets. Update TraceLog.podspec: Set new deployment target versions. removing xcode specific properties from travis.yml fixing project workspace name updating travis.yml Updating debug identifiers to Swift 2.2 syntax
…rite * 'master' of github.com:tonystone/tracelog: - Added performance tests with baselines for Swift and ObjC. - Updated OS X tests to run both Swift and ObjC tests. Update TraceLogTests.swift - Appended _Swift to end of class name to distinguish it in the test output. - Removing unused class Testriter - Changed test method names to be test<func name>. Update TraceLogTests.m - Renaming test class name appending _ObjC to it. - Removing extra tests with string constants. - Changed printed method to the same as Swift accept replacing "Swift" with "ObjC". Update TLLogger.h and .m - Exposed a new method to set the Writers (setWriters). In this version is is mainly for testing.
- Updating 'Core' subspace source_files to only include swift files. - Adding pod_target_xcconfig section for SWIFT_VERSION of 3.0 (required by Xcode 8) - Reformatting xcconfig section.
Current coverage is 100% (diff: 100%)@@ master #23 diff @@
=====================================
Files 13 6 -7
Lines 438 268 -170
Methods 70 0 -70
Messages 0 0
Branches 18 0 -18
=====================================
- Hits 403 268 -135
+ Misses 35 0 -35
Partials 0 0
|
… OS X import "-Swift.h" file
- Changing language back to objective-c since travis does not actually support swift in the language statement. - Updated Matrix to have individual variables so they can be passed to xcodebuild with spaces in them.
- Removed extra = sign after -scheme which is invalid.
if let level = upperCaselogLevelString.asLogLevel() { | ||
self.globalLogLevel = level | ||
} else { | ||
errors.append(.invalidLogLevel("Variable '\(upperCaseVariable)' has an invalid logLevel of '\(upperCaselogLevelString)'. '\(Configuration.logAll)' will be set to \(self.globalLogLevel).")) |
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.
Per this error, how is globalLogLevel
being set to logAll
?
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.
The test coming into this section is specifically for LOG_ALL (you can't enter this section unless that was passed). The self.globalLogLevel
is setup to a default value (currently INFO) at the beginning so if the upperCaselogLevelString
is an invalid value, the following message evaluates to the proper message as in the tests (test message from func testInitialize_LogWriters_Environment_GlobalInvalidLogLevel()
below).
"Variable 'LOG_ALL' has an invalid logLevel of 'TRACE5'. 'LOG_ALL' will be set to INFO."
I have changed the '\(Configuration.logAll)'
to '\(upperCaseVariable)'
statement to be more consistent with the rest of messages though.
|
||
if let level = upperCaselogLevelString.asLogLevel() { | ||
/// | ||
/// Note: in order to allow for case sensitive tag prefix names, we use the variable instead of the uppercase version. |
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 think it would be tough to remember that some strings are case sensitive, and some are not. I would consider making all strings case sensitive.
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.
Hmm, I have had previous incarnations of this all case insensitive and at some point switched to case sensitive but can't recall the justification. @ndrlndr do you remember why I opted for the change?
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.
@rk-yen So the original reason I believe was that I was matching osx, ios and linux case sensitivity in environment variables. Yes, it is slightly inconsistent in that the PREFIX of the variable is turned case insensitive. LOG_PREFIX_ and LOG_TAG_ portions of the variable do ignore case but the and portion respect case. We should make it consistent by either making it all case sensitive or making it not.
The reasoning for the mix was (I believe) to keep the portion that is important to the user (i.e. the Prefix or Tag) sensitive (and also matching visually what a tag or prefix would actually look like (in the case of a class name for instance) and ignore the part of that is our benifit (meaning we require LOG_PREFIX_ and LOG_TAG_ to be able to tell what they are asking us for).
I'm a big proponent of keeping it consistent with the OS (and programming languages) but could be swayed the other way. Thoughts?
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.
@tonystone .... that makes sense, i.e., having the tags themselves be case sensitive, and the rest be case insensitive. I don't have objections with that.
/// Determine the prefixes log level if available | ||
for (prefix, logLevel) in self.loggedPrefixes { | ||
if tag.hasPrefix(prefix) { | ||
level = logLevel |
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.
Maybe the loggedPrefixes
should be traversed in the reverse
direction and then also, maybe we need a break
here?
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.
Yes, both would be good optimizations given the expected usage. Will make the change.
/** | ||
* Called when the logger needs to log an event to this logger. | ||
* | ||
* @param timestamp Timestamp of the log event (number of seconds from 1970). |
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.
This is defined as time from epoch, but ConsoleWriter.swift
and Logger.swift
use this as time from ReferenceDate. When you have to pick one, I would prefer time from epoch (1970) instead of time from referencedate (2001).
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.
Good catch, changed.
/// Note: this enum must support both Swift and ObjC | ||
/// | ||
@objc public enum LogLevel : Int, Comparable { | ||
case off = 0 |
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.
Does a LogLevel off
turn off all logs for that module? If so, we should try to discourage that usage. It would be nice if we can say:
- If logging is ON, then error logs will definitely show up.
- If you don't want error logs, then turn off logging completely.
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.
@rk-yen So remove "off" in leu of "TRACELOG_DISABLED"?
In case that is the idea, consider that "off" was meant for you to be able to turn specific "noisy" classes/tags off so you can focus on specific modules to debug (we probably don't want to lose that). Here's an example of the settings for that.
TraceLog.configure(writers: [MyWriter()],
environment: ["LOG_ALL": "TRACE4",
"LOG_PREFIX_FHA" : "OFF",
"LOG_TAG_TraceLog" : "TRACE4"])
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.
If we do want the ability to disable logs for a particular module, then having this OFF
level is ok.
What I was suggesting is that if logging is enabled, then we do not provide the user to disable ERROR level logs, i.e., a user should never not be interested to see ERROR level logs.
The following example shows that all classes/tags will have INFO level logs, and a specific module will have TRACE4 level logs.
TraceLog.configure(writers: [MyWriter()],
environment: ["LOG_ALL": "INFO",
"LOG_TAG_FHA" : "TRACE4"])
The following example shows that all classes/tags will have ERROR (default) level logs, and a specific module will have TRACE4 level logs.
TraceLog.configure(writers: [MyWriter()],
environment: ["LOG_TAG_FHA" : "TRACE4"])
Just to clarify, the current code is correct in what it is doing. What I am suggesting is that we encourage the proper use of the logging framework, where we never lose ERROR level logs.
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 your point now. Allowing ERROR to be the lowest level does offer value in the framework. I will remove OFF from the dynamic setting relaying on TRACELOG_DISABLED to completely turn logging off.
let staticContext = StaticContextImpl(file: file, function: function, line: line) | ||
|
||
/// | ||
/// All logPrimative calls are asynchronous |
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.
typo
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.
Corrected.
/// | ||
/// - Parameters: | ||
/// - writers: A String to use as a tag to group this call to other calls related to it. If not passed or nil, the file name is used as a tag. | ||
/// - environment: An closure or trailing closure that evaluates to the String message to log. |
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.
copy paste error. Description is from the next method.
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.
Fixed and added example.
* @param message The message string (already formatted) for this logging event. | ||
* @param file The source file (of the calling program) of this logging event. | ||
* @param function The function (of the calling program) which is being called. | ||
* @param lineNumber The source line number (of the calling program) of this logging event. |
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.
These params don't match the function arguments.
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.
Fixed.
547717f
to
e81a940
Compare
Added a vagrant environment for development and testing. Swift PM and Linux Support - Reorganize structure for Swift Package Manager - Finished port to Linux flavors - Added ruby file in Scripts to generate the XCTest files for linux testing. - Added vagrant file for linux dev work. - Rebuilt TraceLog.podspec to deal with Swift PM structure changes. - Made Swift the default version in the podspec - Merged the Swift and the Core subspecs since they are now one. - Changed comment style throughout. - Simplified synchronization strategy. - Removed RecursiveSerialQueue as it's not needed anymore. Update README.md - Adding Swift 3 badge - Changed Platform badge to new Platforms badge adding linux. - Changed license badge and moved to after the project name. Adding codecov.yml configuration file to control coverage reports.
…ut of public log functions.
- Add EnvironmentTests.swift with basic tests for this public class. - Update README.md adding code coverage badge. - Update README.md change target for badges that do not have a direct landing place to point to the tracelog project. - Fixing codecov.yml to fully exclude the Test code. - Adding ConfigurationTests.swift with basic tests to bring coverage up to 100%. - Update .travis.yml adding more iOS test versions and simulators. - Removing iOS 8 builds for the moment as they timeout. The Example application runs but the tests don't seem to start.
…th the same method added to the ObjC TLLogger class.
e81a940
to
d610314
Compare
- Enhance the comments for configure. - Change the message for the logging of the configuration change.
- Moving TraceLogPerformance.m over into the iOS test target since iOS is the most broadly tested on different platforms. - Update TraceLogPerformanceTests.m to use TLLogger where it can rather than the test proxy. This will cover TLLogger configureWithEnvironment in the tests. - Corrected testinitialize method names changing to testIntiailize with uppercase I. - Corrected expected message from testInitialize methods. - Correcting case of #import "TraceLog_IOS_Tests-Swift.h" - Corrected case of initialize in testinitialize changing the I to upper case.
- Separated out ExpectationValues into its own class file. - Update process_test_files.rb to only process classes that are subclasses of XCTestCase. This will avoid processing helper classes. - Ran process_test_files.rb to update linux test runner files and +XCTest extensions. - Added new ConfigurationTest+XCTest.swift and EnvironmentTests+XCTest.swift to project file. Update README.md - Added Swift 3.0 requirement - Added Installation (Swift Package Manager) - Updated Installation (CocoaPods) - Adding clang and lldb requirements for Ubuntu 14.04 Update .travis.yml - Added linux build and test - Added OS X swift package manager build
- Configuration.swift: Changed the '\(Configuration.logAll)' to '\(upperCaseVariable)' statement to be more consistent with the rest of messages though. - Configuration.swift: Changed loggedPrefixes to traverse in the reverse direction and added a break when level is found to optimize the method. - Logger.swift: Changed to use Date().timeIntervalSince1970 instead of timeIntervaleSinceReferenceDate. - ConsoleWriter.swift: Changed to use Date(timeIntervalSince1970: timestamp) instead of Date(timeIntervalSinceReferenceDate: timestamp). - Writer.swift Updated parameter comments to match parameters and added example call. - Logger.swift: Corrected typo on line 127. - TraceLog.swift: Corrected documentation on configure func.
- Cutting down the output of the performance test by reducing the iterations for the output tests to 1000. This is due to an issue on travis which limits the log size to 4m. At that point the process is terminated (see travis-ci/travis-ci#3865). - Updated performance test baselines to match lower iteration count. Note: all tests from version 1.0 to 2.0 have been within tolerance so changing these baselines should keep it relatively the same.
…nstants to TLLogger to handle ObjC log levels.
d610314
to
a2dab54
Compare
This was done to enforce a minimum log level of at leave ERRORs. If logging is to be turned off, it can be done through TRACELOG_DISABLED.
- Updated configuration section adding static configuration via TraceLog.configure. - Modified Configuration section for XCode converting it to environment configuration. - Added section on Configuring log Writers. - Corrected a few typos from previous version.
Due to the using Xcode 8 and Swift 3, TraceLog does not pass `pod lib lint` unless we upgrade to CocoaPods version 1.1.0.rc.2. This version has specific functions to set the Swift version. CocoaPods 1.1.0 requires a .swift-version file this was created as well.
@tonystone .... looks good to me. Only one clarification comment is about the case sensitivity of the tags. |
Does the Cocoapods change require people to use 1.1.0, or does the pod still work with 0.39.0 and 1.0.1? |
@ndrlndr I don't know the answer, that would have to be tested. I believe though that the only way to use XCode 8 and Swift 3 is to move up to this CocoaPods version because of the requirement by Apple. CocoaPods 1.1.0.rc.2 added the .swift-version file to select the version you require for you app. |
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 to see support for Linux, things generally look OK.
Complete Swift rewrite with the ability to pass in the environment and log writers.