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

Introducing UIKit & Cocoa extensions in RxSwitExt (e.g. RxCocoaExt) #124

Closed
freak4pc opened this issue Dec 11, 2017 · 6 comments
Closed

Comments

@freak4pc
Copy link
Member

freak4pc commented Dec 11, 2017

There has been increasing activity and demand for a place where UI-Related components and extensions could live in the RxSwiftCommunity eco-system.

This would help:

  1. Find place for things that are needed by the community but would bloat the main repo (e.g. implementation details not tied to a delegate, etc.)
  2. Get more community involvement, as there is so many options for new extensions to be added here, this might help get more people involved in contributing.
  3. Lower entry-barrier for people who are just starting with Rx and want a "Catch all" for UI-related extensions as well.

After a lot of discussion inside the community and with @fpillet , it seems there are three options here.

  1. Separate library (RxCocoaExt)
  2. Subspec on RxSwiftExt (e.g. RxSwiftExt - RxSwiftExtCore, RxCocoaExt)
  3. Separate podspec(s) in the same repo (RxSwiftExt.podspec, RxCocoaExt.podspec). This would follow the main repo's behavior with RxSwift.podspec, RxCocoa.podspec, RxTest.podspec and RxBlocking.podspec.

The last two options seem best to keep things consolidated, and also have more community involvement since this would be a place where more contributors could be involved. but this is one of the reasons for this discussions.

If we do either option we need to discuss several things here:

  1. Coding standards - we need to have good Swiftlint rules in place
  2. Naming standards - Right now many operators are named camel-case (e.g. mapTo) instead of Class-case (e.g. MapTo). I guess because its operators vs. classes in this case, but we should make a decision on what makes sense and how to name files across different boundaries. (same for Folder structure here)
  3. PR standards - we need to have something like Danger or Peril set-up to avoid spending contributor/maintainer time on filtering PRs that are missing trivial details (tests, playgrounds page, etc).
  4. Framework DIfferences - for example, a Playground page for a UIKit extension might not be relevant. Same for Cocoa on macOS. Tests are also harder when it comes to UI Components.
  5. What goes in - Do we just want to accept any suggestion? Do we want to have a proposal stage (e.g. open an Issue and get clearance from repo owners/collaborators) ?

I'm sure there are many more points to discuss but this would be a great starting point.

I hope to get involvement from @RxSwiftCommunity/contributors, when and if you have some time.

Thanks all!
Shai 🎉

@bobgodwinx
Copy link
Member

bobgodwinx commented Dec 11, 2017

@freak4pc I just gave a first reading to this issue and IMHO I think a new library will be better because we are just going to be extending what is missing from RxCocoa so there is no need mixing thing with RxSwiftExt so I would go with first.

  1. Separate library (RxCocoaExt)

Secondly we must apply a strict rule that this is UI-related stuffs which are missing from RxCocoa

Coding standards - we need to have good Swiftlint rules in place

Swiftlint is welcomed I really like it so it should be a rule

Naming standards - Right now many operators are named camel-case (e.g. mapTo) instead of

I will stick will camel-case because it creates more code harmony and most of us are coming from Obj-C and this was a standard there. However we could look into this style

for example, a Playground page for a UIKit extension might not be relevant

I totally agree but there should be a way to test what is going in.

we need to have something like Danger.

I totally agree to this also.

Finally I would suggest we go for a git workflow where master <- develop <- feature-branch
and always leave master holy.

@fpillet
Copy link
Member

fpillet commented Dec 11, 2017

I don't mind either way, I'm easy. I think most users don't mind either, there's not enough volume of code in the single RxSwiftExt repo to qualify as "bloat" -- a separate repo would mean one more pod, and one more framework in applications, which adds to the startup time.

Aside from this, a separate repo could make discovery easier for users (but then maybe it's just a matter of building good documentation).

THIS SAID. Some of the points raised are perfectly valid. Regardless of whether we make a separate repo, there are better practices we could put in place right now that would help improve quality (which is already high, IMHO) and ease interaction with contributors.

Coding standards - we need to have good Swiftlint rules in place

Yes possibly. I usually don't use such tools as I think all they do is fulfil a narcissistic vision of the code, but why not. I'm not oppose to having a linter if it makes code easier to read for everyone.

Naming standards - Right now many operators are named camel-case (e.g. mapTo) instead of Class-case (e.g. MapTo)

And for good reason. Operators are not classes, and they are camel-cased in RxSwift itself. I don't see what uppercasing would bring aside from confusion. There are things NOT camel-cased in RxSwift (i.e. test file names), I'll fix this. We need consistency, I agree.

PR standards - we need to have something like Danger or Peril set-up

Yes! Totally worth it.

Framework DIfferences - for example, a Playground page for a UIKit extension might not be relevant.

Not sure about this one. The main issue with Playground (especially when it comes to UI playgrounds) is that they are unreliable. OTOH, it's easy and relatively painless to make a page with a quick demo in a playground, getting straight to the point in a contained example. A full-fledged application is harder to come by (if you want to have one app to show all features).

What goes in - Do we just want to accept any suggestion? Do we want to have a proposal stage

Totally favoring the proposal-first approach. I highlighted steps for RxSwift in CONTRIBUTING.md document I recently updated there, I think we should follow a similar discipline.

I really appreciate the exceptional attention to detail that you @freak4pc put into your PR reviews. It bumped the standard of what a proper review should be, as you tend to go very deep in the details. It's probably going to be difficult to keep the same level of attention with more reviewers, but this should be part of the "best effort" towards quality.

@bobgodwinx
Copy link
Member

I don't mind either way, I'm easy. I think most users don't mind either, there's not enough volume of

@fpillet I did mention about a fresh repo because it would be totally be a focused concept of realizing all the things that one would need when working on UI that is the promise of this repo if we go for it. I am not a fan of mixing pure RxSwift related with RxCocoa stuffs. I think discoverability is not a problem, people will definitely see it once published.

Not sure about this one. The main issue with Playground (especially when it comes to UI playgrounds)

@fpillet let's say that I find it really hard with playgrounds sometime, but this is not a conclusion that it doesn't work. I think we can defer this for now.

@freak4pc @fpillet if all good I can set-up the repo with Swift-lint, Circle-CI, CocoaPods and Carthage. I have limited knowledge on Danger I haven't used the tool.

@mosamer
Copy link

mosamer commented Dec 12, 2017

Thanks @freak4pc for the detailed issue, there's lot to tackle here. If you don't mind, I'd like to break this down into 3 parts with potential to generalize a couple of them to the overall RXSwiftCommunity organization.

This would help:

  1. Find place for things that are needed by the community .......

I think this is a very good seed for an organization-wide Vision or Manifest document. I believe it will be useful to have such a document describing the vision and motivation behind RxSwiftCommunity without guidelines or rules directly included but rather to be derived from if I'd say.

If we do either option we need to discuss several things here:

Coding standards - we need to have ........

Again, I can see a benefit from generalizing this to be organization wide -with few exceptions of course-. If agreed, I'd suggest starting a team discussion for each item of those and try to settle on a greatest common divisor across all repos and gradually apply that.

Moving to RxSwiftExt specific matters :) regarding how to introduce RxCocoaExt;

;TLDR; I am totally against the first option, while I think the other two options are equally good with the second is better from users perspective (but adds burden to maintainers) and third is simplest as it builds on existing conventions.

  1. Separate library (RxCocoaExt)
  • I don't think this option is either viable or maintainable.
  • RxCocoa is not limited to UIKit extensions and so will be RxCocoaExt. This means custom operators should be introduced to both simultaneously (this is already the case with mapTo, distinct and not for instance).
  • Separating repos will render sharing code almost impossible not to mention the extra process cost, for instance if bug was found in one of these operators where should an issue be opened? what is the process to apply a fix?
  1. Subspec on RxSwiftExt
  • it is still a single pod! with the increasing worry on number of dependencies and launch time this might be a plus.
  • For new users, the default behavior of all-included should be fine for discoverability. More advanced users can have a granular control over what to include.
  • If we adopted this approach, I suggest divide the subspecs based on their dependencies. E.g.
    • RxSwiftExt/Core - RxSwift extensions
    • RxSwiftExt/RxCocoa - RxCocoa extensions (types & operators)
    • RxSwiftExt/Foundation - Foundation extensions (e.g. URLSession, Codable, etc)
    • RxSwiftExt/UIKit - UIKit extensions (e.g. UIScrollView, etc)
    • etc
  • As maintainers, this may add the burden on deciding what goes where and what should be even introduced.
  1. Separate podspec(s) in the same repo.
  • It builds up on the established conventions of the main repo and simply mirror them. This is true for maintainers as well as users.
  • The only caveat, which is really minor, is that all different podspecs will have to have the same version/releases. Not a big deal and again it is the same case with main repo, so if we are happy with this we should be with that 🤷‍♂️

@freak4pc
Copy link
Member Author

freak4pc commented Dec 12, 2017

Thanks everyone for all your opinions :)

I'll try to address everything
I'll start at the end -
My current instinct is going with two podspecs in the same Repo.

The other two approaches are too prone to issues:

  1. Separate repo - All points @mosamer made are pretty much my thoughts. There's no really good reason to create that sort of separation and since many of these components might share some capabilities (across Swift/Cocoa boundaries), creating full separation would be hard and painful.

  2. Subspecs - This was my original thought but I have grown against it 😄 main reasons are - It's not in sync with how the main Repo works, and the separation to Subspecs could get too granular (for example @mosamer you wrote RxCocoa separated from UIKit, which would be incorrect as you RxCocoa should support UIKit, macOS and tvOS components, potentially).

Regarding the caveat you mentioned @mosamer - I don't think it's a huge deal. But TBH you don't have to do this. They are separate specs, so you could have separate versioning, but it would make it much harder to reason for and track so you probably wouldn't want to 😉

@fpillet regarding some of your points

that would help improve quality (which is already high, IMHO)

It's definitely high. But my long-term ambition is to get MANY more people involved... and to make that effort scalable we need to assume we're going to get some junk PRs, so the more we're standardized, the easier this is gonna be :P

Yes possibly. I usually don't use such tools as I think all they do is fulfil a narcissistic vision of the code, but why not. I'm not oppose to having a linter if it makes code easier to read for everyone.

I get where you're coming from but the repetitively of commenting on trivial code styling issues is pointless and spends precious (and limited) contributor time. I'm all up for automation to try and reduce this stress so we can concentrate mainly on code and functionality reviews 🥇 We can try it out and see if it's a pain or not, and adjust as we go.

And for good reason. Operators are not classes, and they are camel-cased in RxSwift itself.

That's exactly what I wrote the next line 😄 I just wanted to confirm, and yes having uniformity is what I would aim for in general. Operators should be camelCase where if you build an entirely new class you would probably want to make it ClassCase.

It's easy and relatively painless to make a page with a quick demo in a playground, getting straight to the point in a contained example.

That's true. Actually no problem with enforcing Playgrounds for every PR / new addition 👍

I really appreciate the exceptional attention to detail that you @freak4pc put into your PR reviews. It bumped the standard of what a proper review should be, as you tend to go very deep in the details. It's probably going to be difficult to keep the same level of attention with more reviewers, but this should be part of the "best effort" towards quality.

Really really appreciate the kind words, especially coming from you 🙏
I think it's hard keeping high PR standards always. Even the most experienced will miss a few details, which is why I'm so much in favor of automation, so we can concentrate on code quality and not on minor nitpicks (as much as possible).

I'm going to compile a list of some Action Items so we don't discuss this to much and actually just GTD ! 💪💪💪💪💪

@freak4pc
Copy link
Member Author

Continuing Action Items discussion at #127

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

No branches or pull requests

4 participants