Swift rewrite #23

tonystone · 2016-10-02T02:22:20Z

Complete Swift rewrite with the ability to pass in the environment and log writers.

…w required).

- 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.

codecov-io · 2016-10-02T02:48:42Z

Current coverage is 100% (diff: 100%)

Merging #23 into master will increase coverage by 7.99%

@@             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

Powered by Codecov. Last update ec682a8...5e55de5

… 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.

rk-yen · 2016-10-07T22:13:05Z

Sources/TraceLog/Configuration.swift

+ 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)."))


Per this error, how is globalLogLevel being set to logAll?

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.

rk-yen · 2016-10-07T22:15:48Z

Sources/TraceLog/Configuration.swift

+
+ 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.


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.

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?

@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?

@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.

rk-yen · 2016-10-07T22:21:43Z

Sources/TraceLog/Configuration.swift

+ /// Determine the prefixes log level if available
+ for (prefix, logLevel) in self.loggedPrefixes {
+ if tag.hasPrefix(prefix) {
+ level = logLevel


Maybe the loggedPrefixes should be traversed in the reverse direction and then also, maybe we need a break here?

Yes, both would be good optimizations given the expected usage. Will make the change.

rk-yen · 2016-10-07T22:33:42Z

Sources/TraceLog/Writer.swift

+ /**
+ * Called when the logger needs to log an event to this logger.
+ *
+ * @param timestamp Timestamp of the log event (number of seconds from 1970).


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).

Good catch, changed.

rk-yen · 2016-10-07T22:41:37Z

Sources/TraceLog/LogLevel.swift

+/// Note: this enum must support both Swift and ObjC
+///
+@objc public enum LogLevel : Int, Comparable {
+ case off = 0


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.

@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"])

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.

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.

rk-yen · 2016-10-07T22:44:36Z

Sources/TraceLog/Logger.swift

+ let staticContext = StaticContextImpl(file: file, function: function, line: line)
+
+ ///
+ /// All logPrimative calls are asynchronous


rk-yen · 2016-10-07T22:48:21Z

Sources/TraceLog/TraceLog.swift

+///
+/// - 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.


copy paste error. Description is from the next method.

Fixed and added example.

rk-yen · 2016-10-07T22:53:40Z

Sources/TraceLog/Writer.swift

+ * @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.


These params don't match the function arguments.

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.

- 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.

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.

rk-yen · 2016-10-11T04:18:54Z

@tonystone .... looks good to me. Only one clarification comment is about the case sensitivity of the tags.

ndrlndr · 2016-10-11T16:24:39Z

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?

tonystone · 2016-10-11T16:29:09Z

@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.

ndrlndr

Nice to see support for Linux, things generally look OK.

tonystone added 17 commits May 16, 2016 10:51

Initial Swift rewrite (WIP).

5d468e7

Update travis.yml to update to using the Xcode 7.3 image.

6324a24

Update README.md to reflect changes in required Xcode version (7.3 no…

0f62281

…w required).

Complete rewrite of the core in Swift 3.

e46aec2

Added TraceLog.playground to the project.

d63d57f

Update TLAppDelegate removing commented out import.

6f55280

Added TraceLog.playground to project file.

59ac470

Update Configuration.swift

cb0f532

- 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.

Update Logger.swift

0386ec5

- Adding more comments - Added new tag for Configuration.logLevel func.

Update .travis.yml correcting osx_image tag. Was xcode8.0 now is xcode8.

34101a0

Update TraceLog.podspec

09baf96

- 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.

Ran pod update to update project files.

b5f5f64

Adding LoggerProxy.swift to setup performance tests correctly in ObjC.

5aaa20c

Update README.md changing Xcode version to 8.0.

3401ac6

Updating .travis.yml adding destination param to iOS Xcodebuild command.

cc52490

tonystone added 5 commits October 1, 2016 21:15

Update project file to remove build error on iOS which can;t find the…

3576b9c

… OS X import "-Swift.h" file

Update .travis.yml correcting -destination OS changing it to 10.0

b4f36a0

Adding linux swift build and vagrant to the git ignore.

6fd29ad

Update .travis.yml

6165563

- 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.

Update .travis.yml

03c63db

- Removed extra = sign after -scheme which is invalid.

rk-yen reviewed Oct 7, 2016

View reviewed changes

tonystone force-pushed the swift-rewrite branch 2 times, most recently from 547717f to e81a940 Compare October 9, 2016 17:05

tonystone added 4 commits October 9, 2016 13:19

Moving TLLogger back into the Logger file.

171fbcb

Enhancing TraceLogTests.swift adding asynchronous validation for outp…

3347bca

…ut of public log functions.

Added TLLogger back into Logger.swift.

64b9f6e

tonystone added 6 commits October 9, 2016 13:22

Fixed printing of Configuration Errors.

bf8618a

Correcting misc typo's.

3abe906

Adding import for Swift to Envrionments.swift.

de04742

Requiring first argument for logPrimitive.

78ec9bd

Changing TraceLog.initialize to TraceLog.configure to not conflict wi…

693eeb4

…th the same method added to the ObjC TLLogger class.

tonystone force-pushed the swift-rewrite branch from e81a940 to d610314 Compare October 9, 2016 20:25

tonystone added 10 commits October 9, 2016 13:40

Update Logger.swift

a92298e

- Enhance the comments for configure. - Change the message for the logging of the configuration change.

Changing to use TRACELOG_DISABLED instead of DEBUG and TRACELOG_ENABLED.

56242aa

Upgraded to Xcode recommended settings.

ba2037d

Changed playground to mac.

f394723

Converting LogLevel to pure swift class (removing @objc) and added co…

c64dd90

…nstants to TLLogger to handle ObjC log levels.

Optimizing log calls by removing some layers of func calls and inlining.

a2dab54

tonystone force-pushed the swift-rewrite branch from d610314 to a2dab54 Compare October 9, 2016 20:43

tonystone added 5 commits October 9, 2016 14:51

Update README.md adding clang and lldb command.

ad81c6f

Removing LogLevel.off as an option.

d0cdaf0

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.

Update README.md

8252ad0

- 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.

Corrected 2 typos in README.md.

554de98

rk-yen approved these changes Oct 11, 2016

View reviewed changes

ndrlndr approved these changes Oct 11, 2016

View reviewed changes

tonystone merged commit d08c2b5 into master Oct 11, 2016

tonystone deleted the swift-rewrite branch October 11, 2016 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swift rewrite #23

Swift rewrite #23

tonystone commented Oct 2, 2016

codecov-io commented Oct 2, 2016 •

edited

Loading

rk-yen Oct 7, 2016

tonystone Oct 8, 2016 •

edited

Loading

rk-yen Oct 7, 2016

tonystone Oct 8, 2016

tonystone Oct 11, 2016

rk-yen Oct 11, 2016

rk-yen Oct 7, 2016

tonystone Oct 8, 2016

rk-yen Oct 7, 2016

tonystone Oct 8, 2016 •

edited

Loading

rk-yen Oct 7, 2016

tonystone Oct 8, 2016 •

edited

Loading

rk-yen Oct 8, 2016

tonystone Oct 10, 2016

rk-yen Oct 7, 2016

tonystone Oct 8, 2016

rk-yen Oct 7, 2016

tonystone Oct 8, 2016

rk-yen Oct 7, 2016

tonystone Oct 8, 2016

rk-yen commented Oct 11, 2016

ndrlndr commented Oct 11, 2016

tonystone commented Oct 11, 2016

ndrlndr left a comment

Swift rewrite #23

Swift rewrite #23

Conversation

tonystone commented Oct 2, 2016

codecov-io commented Oct 2, 2016 • edited Loading

Current coverage is 100% (diff: 100%)

Choose a reason for hiding this comment

tonystone Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonystone Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonystone Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rk-yen commented Oct 11, 2016

ndrlndr commented Oct 11, 2016

tonystone commented Oct 11, 2016

ndrlndr left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 2, 2016 •

edited

Loading

tonystone Oct 8, 2016 •

edited

Loading

tonystone Oct 8, 2016 •

edited

Loading

tonystone Oct 8, 2016 •

edited

Loading