freshOS / Arrow

🏹 Parse JSON with style
MIT License
387 stars 27 forks source link

Several improvements #17

Closed maximkhatskevich closed 8 years ago

maximkhatskevich commented 8 years ago
s4cha commented 8 years ago

Hi @maximkhatskevich,

First of all I would like to thank you for the time you took to put up this quite massive PR, and let you know that I am sorry for taking so long to reply.

This is full of nice improvements indeed :)

Unfortunately the code formatting has changed all over the place which is why it cannot be merged "as is".

From now on, we are going to rely on https://github.com/realm/SwiftLint formatting so that everyone stands on the same ground.

Apart from that, I really like the api improvements you suggest.

@maxkonovalov do you have any thoughts on that ?

Cheers,

maxkonovalov commented 8 years ago

Hey guys, I agree that this PR is a good addition to the lib, but I believe that it needs some tweaks before merging:

On the ArrowInitializable protocol I think it's a good addition, but what do you guys think of changing the <--/ operator to <== or something similar? (the latter was used in early Arrow versions).

maximkhatskevich commented 8 years ago

Okay, guys. I'll fix formatting, thanks for the feedback!

On Aug 6, 2016, at 10:05, Max Konovalov notifications@github.com wrote:

Hey guys, I agree that this PR is a good addition to the lib, but I believe that it needs some tweaks before merging:

I prefer the original code formatting, the proposed one has too much linebreaks that don't make it easier to read but do the contrary and almost double the lines count. Besides, it doesn't look Swift-style but rather C++ or something like this. So I would suggest discarding the styling additions completely. The same concerns the //=== and // === MARK: comments - they do not add any help and should be replaced with corresponding // MARK: XXX, // MARK: - XXX or // MARK: - XXX - variants, which are handled correctly by Xcode's jump bar. On the ArrowInitializable protocol I think it's a good addition, but what do you guys think of changing the <--/ operator to <== or something similar? (the latter was used in early Arrow versions).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

s4cha commented 8 years ago

Hi @maximkhatskevich,

Your initial idea about the ArrowInitializable protocol has been implemented separately in https://github.com/freshOS/Arrow/pull/28

Thanks again for your work,

Closing this for now

maximkhatskevich commented 8 years ago

Great, thanks!