Skip to content
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

Several improvements #17

Closed
wants to merge 13 commits into from
Closed

Several improvements #17

wants to merge 13 commits into from

Conversation

maximkhatskevich
Copy link

  • Minor bug fix ('collection' property wasn't available because it wasn't marked as 'public').
  • Extended functionality (see 'ArrowInitializable' protocol and '<--/' infix operator).
  • Improved code formatting to make it easy to read.
  • Few minor code cleanups.

so it's easier to read and reason about.
JSON.collection wasn't marked as public
Plus minor class reorganization to increase readability
- 'data' property does NOT need to be optional, as we do not create class instance at all if deserialized data represetation is nil;
- 'init' should not be optional, as it will simplify the code and increase readability, while usually input data is being checked outside of the class anyway.
Warning: no unit tests so far.
@s4cha
Copy link
Member

s4cha commented Aug 6, 2016

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
Copy link
Member

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

@maximkhatskevich
Copy link
Author

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.

@maxkonovalov maxkonovalov mentioned this pull request Nov 7, 2016
@s4cha
Copy link
Member

s4cha commented Nov 7, 2016

Hi @maximkhatskevich,

Your initial idea about the ArrowInitializable protocol has been implemented separately in #28

Thanks again for your work,

Closing this for now

@s4cha s4cha closed this Nov 7, 2016
@maximkhatskevich
Copy link
Author

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants