Implement JSONWriter to better encapsulate writing JSON content to file. #10

andrea-prearo · 2017-04-26T16:56:31Z

This PR addresses #9.

Changes: - Upgrade project to Swift 3.1. - Embed Swift standard libraries as needed.

Create JSONDataWriter which will validate JSON before writing it to the file. And use it in modules for JSON writing instead of CLUDataWriter. New classes: - JSONWriter: Writes JSON content to file. - JSONWriterError: Defines error related to writing JSON content to file.

Use JSONDataWriter instead of CLUDataWriter to write JSON content to file.

Make ClueExampleApp and ClueSwiftExampleApp shared schemes.

Changes: - Update append to return a Bool value. - Better error handling. - Add new line after each JSON entry.

andrea-prearo · 2017-04-26T18:11:44Z

@Geek-1001 It look like Travis CI is configured to work with Xcode 8.2:

I started working with Xcode 8.3.2, but I can downgrade if needed. Or we could think about upgrading the CI. Whichever you think is better.

Changes: - JSONWriter append return written bytes count as Int. - Update unit tests.

Geek-1001 · 2017-04-27T08:17:39Z

Hi @andrea-prearo
Great job, thank you! 👍

I have some comments regarding particular commits, please check them out. (I'll leave them in 10 minutes or so)

Also regarding CI:
I'm using Xcode 8.3.2 as well. As far as I know, Travis can't use Xcode 8.3.2 image for some bureaucratic reasons, so the latest image available is Xcode 8.2. I sure it's a problem with my custom build script because it was configured specifically for objective-c. Now we have Swift code as well, so I need to setup this in travis.yml file.
I'll fix this one.

andrea-prearo · 2017-04-27T15:13:59Z

Thanks @Geek-1001!

I am using Travis CI for a 100% Swift project without issues. I haven't update it to Xcode 8.3.2 yet, though. I will give it a try sometime this week.

Geek-1001 · 2017-04-28T10:04:39Z

Hi @andrea-prearo
I've read a little bit more about our Travis CI problem.
Here is what the log says:

“Use Legacy Swift Language Version” (SWIFT_VERSION) is required to be configured correctly for targets which use Swift. Use the [Edit > Convert > To Current Swift Syntax…] menu to choose a Swift version or use the Build Settings editor to configure the build setting directly.

I'm sure now this was caused by "Xcode 8.2" in our build script. Since you've specified Swift 3.1 in the project and Xcode 8.2 doesn't have it according to Swift releases page

So to fix this issue you just need to update .travis.yml file.
And change osx_image to xcode8.3 Travis: available Xcode images

osx_image: xcode8.3

and OS version in the build script to OS=10.3 Travis: available SDKs for Xcode 8.3

script:
  - xcodebuild test -project Clue.xcodeproj -scheme ClueTests -destination "platform=iOS Simulator,name=iPhone 7,OS=10.3" CODE_SIGN_IDENTITY="" CODE_SIGNING_REQUIRED=NO ONLY_ACTIVE_ARCH=NO

I've tested this script locally and everything was fine.

Changes: - Update source file header comments.

Changes: - Update Travis CI to Xcode 8.3. - Update script to start iOS10.3 simulator.

andrea-prearo · 2017-04-28T12:19:33Z

@Geek-1001 Thanks for the detailed info on updating Travis CI!

Geek-1001 · 2017-04-29T19:53:53Z

Hi @andrea-prearo
Do you have some comments regarding my notes in your commits? 🙂
I'd like to merge your changes to be able to start Swift migration of VideoWriter from #11

Thanks ahead!

andrea-prearo · 2017-04-29T21:30:55Z

Hi @Geek-1001,

GitHub is not showing me your notes. It looks like this PR has only 6 comments (7 with this one). Could you please post them again?

Feel free to merge this PR. I will address any feedback you may have and create a new PR for that.

Thanks!

Geek-1001 · 2017-04-27T08:37:23Z

Clue/Classes/Writers/JSONWriter.swift

+
+import Foundation
+
+public class JSONWriter: NSObject {


I saw that you removed CLUDataWriter in the next commit so maybe my comment is already irrelevant, but what do you think about the following case:
What if we can subclass JSONWriter from CLUDataWriter
This will leave an ability for some modules to write some raw data (just a string, for instance) into the file if needed and JSONWriter would be just a layer on top of DataWriter. That's mean we can migrate CLUDataWriter > DataWriter to Swift as well. Also, this will allows us to encapsulate all stream related operations into the DataWriter so JSONWriter would deal only with high-level writing and JSON validation.

I believe this approach would leave some flexibility for output data and another layer of abstraction would be easier to understand.

What do you think?

Makes sense. I can create a new PR to migrate CLUDataWriter to Swift (as DataWriter) and use it as the base class for JSONWriter (as you suggested). Does this work for you?

Yes, absolutely. 👍

Geek-1001 · 2017-04-27T08:40:08Z

Clue/Classes/Writers/JSONWriter.swift

+    fileprivate let outputStream: OutputStream
+    fileprivate var currentError: JSONWriterError?
+
+    public var error: JSONWriterError? {


What do you think about adding Header Docs for all public methods and variables in JSONWriter?
So we can regenerate documentation (using Jazzy) with this new class as well.

Sure thing. I will add the documentation in the next PR.

Geek-1001 · 2017-04-27T08:51:37Z

Clue/Classes/Protocols/CLUInfoModule.h

 @return New instance of info module
 */
- (instancetype)initWithWriter:(id <CLUWritable>)writer;
+- (instancetype)initWithWriter:(JSONWriter *)writer;


Could you please explain why you decided to change -initWithWriter: argument from CLUWritable to JSONWriter as a default writer for all Info Modules?

I think we should leave those protocols as a general interface so final info module will decide what kind of writer it requires. That's why I used a CLUWritable so final Info Module could use DataWriter, JSONWriter or even VideoWriter

Also, I didn't found same changes in CLURecordableModule that's why I was a bit confused 🙂
What do you think?

I didn't touch CLURecordableModule because I wanted to restrict the changes in this PR to writing JSON data.

On second thought, though, making these changes to CLUInfoModule.h is not needed. I will revert them, and re-introduce id <CLUWritable>, in the next PR.

Geek-1001 · 2017-04-29T21:40:14Z

@andrea-prearo Oh, it's my bad. I forgot to publish my comments so you can see them, sorry 🙂
Now everything should be fine.

Geek-1001 · 2017-04-29T21:45:40Z

Feel free to merge this PR. I will address any feedback you may have and create a new PR for that.

Make sense. I'll merge this one.
And we'll discuss some small details regarding my notes here.
Later we can always create the new pull request if needed.

andrea-prearo · 2017-04-30T00:24:33Z

Thanks @Geek-1001!

I will address your feedback in the next PR.

andrea-prearo added 5 commits April 26, 2017 07:44

Update Xcode project settings to support Swift development

c2d5b84

Changes: - Upgrade project to Swift 3.1. - Embed Swift standard libraries as needed.

Swap CLUDataWriter with JSONWriter

3f55229

Use JSONDataWriter instead of CLUDataWriter to write JSON content to file.

Update shared schemes

4d8c219

Make ClueExampleApp and ClueSwiftExampleApp shared schemes.

Refactor JSONWriter

1c2988f

Changes: - Update append to return a Bool value. - Better error handling. - Add new line after each JSON entry.

Update JSONWriter append method return type

d1cf51c

Changes: - JSONWriter append return written bytes count as Int. - Update unit tests.

andrea-prearo added 2 commits April 28, 2017 05:04

Fix comments

50de9db

Changes: - Update source file header comments.

Update Travis CI config file

2f8d110

Changes: - Update Travis CI to Xcode 8.3. - Update script to start iOS10.3 simulator.

Geek-1001 reviewed Apr 29, 2017

View reviewed changes

Geek-1001 merged commit 7bb6f8a into Geek-1001:develop Apr 29, 2017

andrea-prearo mentioned this pull request Apr 30, 2017

Create DataWriter as a base class for JSONWriter #16

Merged

andrea-prearo deleted the json-writer branch May 7, 2017 03:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement JSONWriter to better encapsulate writing JSON content to file. #10

Implement JSONWriter to better encapsulate writing JSON content to file. #10

andrea-prearo commented Apr 26, 2017

andrea-prearo commented Apr 26, 2017

Geek-1001 commented Apr 27, 2017

andrea-prearo commented Apr 27, 2017 •

edited

Loading

Geek-1001 commented Apr 28, 2017

andrea-prearo commented Apr 28, 2017

Geek-1001 commented Apr 29, 2017

andrea-prearo commented Apr 29, 2017

Geek-1001 Apr 27, 2017

andrea-prearo Apr 30, 2017

Geek-1001 Apr 30, 2017

Geek-1001 Apr 27, 2017

andrea-prearo Apr 30, 2017

Geek-1001 Apr 27, 2017

andrea-prearo Apr 30, 2017

Geek-1001 commented Apr 29, 2017 •

edited

Loading

Geek-1001 commented Apr 29, 2017 •

edited

Loading

andrea-prearo commented Apr 30, 2017

Implement JSONWriter to better encapsulate writing JSON content to file. #10

Implement JSONWriter to better encapsulate writing JSON content to file. #10

Conversation

andrea-prearo commented Apr 26, 2017

andrea-prearo commented Apr 26, 2017

Geek-1001 commented Apr 27, 2017

andrea-prearo commented Apr 27, 2017 • edited Loading

Geek-1001 commented Apr 28, 2017

andrea-prearo commented Apr 28, 2017

Geek-1001 commented Apr 29, 2017

andrea-prearo commented Apr 29, 2017

Geek-1001 Apr 27, 2017

Choose a reason for hiding this comment

andrea-prearo Apr 30, 2017

Choose a reason for hiding this comment

Geek-1001 Apr 30, 2017

Choose a reason for hiding this comment

Geek-1001 Apr 27, 2017

Choose a reason for hiding this comment

andrea-prearo Apr 30, 2017

Choose a reason for hiding this comment

Geek-1001 Apr 27, 2017

Choose a reason for hiding this comment

andrea-prearo Apr 30, 2017

Choose a reason for hiding this comment

Geek-1001 commented Apr 29, 2017 • edited Loading

Geek-1001 commented Apr 29, 2017 • edited Loading

andrea-prearo commented Apr 30, 2017

andrea-prearo commented Apr 27, 2017 •

edited

Loading

Geek-1001 commented Apr 29, 2017 •

edited

Loading

Geek-1001 commented Apr 29, 2017 •

edited

Loading