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

[WIP] RxSwiftExt & RxCocoaExt Action Checklist #127

Open
3 of 14 tasks
freak4pc opened this issue Dec 12, 2017 · 20 comments
Open
3 of 14 tasks

[WIP] RxSwiftExt & RxCocoaExt Action Checklist #127

freak4pc opened this issue Dec 12, 2017 · 20 comments
Assignees

Comments

@freak4pc
Copy link
Member

freak4pc commented Dec 12, 2017

This continues discussion from #124, but decided it would be better to start a clean one since this is already "Calls to Action".

I'm assigning myself as I'd like to do many of the things here myself, but would love help on things related to Carthage and SPM since that's not my expertise :)

Here's the current checklist I have in mind.
Please feel free to comment and help me perfect this before I get started on making some work - hopefully before the weekend.

RxSwiftExt + RxCocoaExt (Single repo) checklist

  • Suggest folder structure for all components (Source/RxCocoa is great, but do we want more internal folders to separate ControlEvents, Operators, etc?)

    Something like this might work (just a suggestion of course needs more discussion)

      - RxSwift
          - Operators
          - Common (or Core or Observables or something to describe non-operators)
      - RxCocoa
          - Operators
          - ControlEvents (or `UI`, or `Components` or something to describe non-operators related to UIKit/Cocoa extensions)
      - Common (rename Tools, just my personal preference Tools are more like scripts and not Helpers or Common shared code) - Common helpers that support both Pods
    
  • Create a new RxCocoaExt subspec that would match the Source paths described above

  • File name convention - Proposed and approved in Decide on File naming for RxCocoaExt(s) and RxSwiftExt(s) #131

  • Swiftlint - Going to add the base and then make custom adjustments based on our current code base, and will refine from there [Swiftlint #128]

  • Danger - Danger is super crucial in getting good automation and quality of PRs IMO. The current pieces I'm thinking of (ideas, not all are necessities)

    • Body must be longer than 25-50 characters (has a normal description) (Should warn if not)
    • CHANGELOG must have been modified (Should error if not)
    • Confirm the Tests folder changed and a relevant test-file was modified/created for a PR (Should error if not)
    • Confirm file name convention for tests, operators, etc.
    • Confirm a Playgrounds page was created if a new file was created (Should error if not)
    • Confirm Playgrounds were modified if Sources were modified (should warn if not)
    • Confirm correct filename if a new operator or control event is added (we know this based on the folder structure and a new function added, possibly), should error if not
  • Migrate to CircleCI - TL;DR Travis is slow and have been crappy for OSS. Myself and @ashfurrow migrated some repos to CircleCI and it's proven to be much more stable and deliver better build times and less waits. (Move to CircleCI #129)

  • Confirm support for Carthage and SPM

@freak4pc freak4pc self-assigned this Dec 12, 2017
@freak4pc
Copy link
Member Author

cc/ @fpillet @mosamer @bobgodwinx

@mosamer
Copy link

mosamer commented Dec 12, 2017

In such a case, will it make sense to move RxTestExt here as well? or would you prefer to wait a little bit on that?

@freak4pc
Copy link
Member Author

Let's wait a bit and start with this first :)
There's a lot of work to be done and this is still WIP

@mosamer
Copy link

mosamer commented Dec 12, 2017

I get it, just some of the items in the list may not be applicable there (Playgrounds for instance) so just to keep that in mind if the plan for this is to happen eventually.

Also, feel free to let me know if you need help with any item in this list, will be glad to help ;)

@freak4pc
Copy link
Member Author

Opinions on Folder Structure and general feedback would be helpful :)
When reaching the end of the list probably ensuring SPM and Carthage behave would help as well 😄

@mosamer
Copy link

mosamer commented Dec 12, 2017

  • I am not sure about subfolders inside RxSwift & RxCocoa
  • I'd keep Common folder for common/shared implementation code
  • I'd add Utils -or Helpers- folder for helper functions shared (currently Tools)

@bobgodwinx
Copy link
Member

After taking a look at both the current RxSwiftExt repo I did also checked how RxSwift repo which contains the RxCocoa relates. I still suggest we go for option one. A new repository because it will actually simplify things. And if we need operators from the RxSwiftExt we can always add it as a dependency just like RxSwift + RxCocoa. If you take a look at the RxSwift repository the only thing that RxCocoa shares is the workspace. The both of them are two different projects and I think the decision of having all of them is one single repository is because ReactiveX gave one repo per language and that is why we find the both in one repo. But if we go for a different one it is specific and clear from the beginning what it is for and why. Because Sometimes I get confused while navigating through the RxSwift repo by associating some operators.

@freak4pc
Copy link
Member Author

freak4pc commented Dec 12, 2017

I still (personally) don't find a case for separating the repos since they are so coupled. I understand your argument regarding ReactiveX single-repo, but I think it does make sense in that context; everything is found in one place, and everything can be maintained by a dedicated group of people without jumping between projects that share so much in common.

@mosamer mosamer mentioned this issue Dec 12, 2017
@freak4pc freak4pc changed the title RxSwiftExt & RxCocoaExt Action Checklist [WIP] RxSwiftExt & RxCocoaExt Action Checklist Dec 12, 2017
@ashfurrow
Copy link
Member

Cool, all sounds good to me 👍 I like the subspecs idea but Carthage users will miss out, that's okay. I also like keeping things in one repo – just from experience I've found it easier. But 🤷‍♂️

@fpillet
Copy link
Member

fpillet commented Dec 12, 2017

Hrmm Carthage users. Good point @ashfurrow, so we need to make separate podspecs instead of subspecs, otherwise we'll be caught in endless support issues with Carthage users, right?

@freak4pc
Copy link
Member Author

My original thought was two separate podspecs in general, for Carthage reasons and other reasons that might not make this the best option

@reloni
Copy link
Member

reloni commented Dec 13, 2017

I'm not a cocoapods user and I don't actually know how this magic work, but carthage simply builds all schemes inside project/workspace, so maybe if we create proper project/workspace it would work regardless podspecs/subspecs.

@ashfurrow
Copy link
Member

Actually the issue with Carthage isn't that bad; Carthage would continue to behave as it does today, but CocoaPods users would be able to specify the subset of the functionality they want. Carthage doesn't have the concept of subspecs, so they'd continue to build all of the files into the framework. It only becomes an issue if one of the subspecs has a strange dependency because then Carthage would have no choice but to build and link against that dep too (this is the case in Moya's RxSwift subspec).

If breaking out the library into subspecs is something we want (as admitted CocoaPods users) then we should go forward with it. If Carthage users feel they're missing out, we should encourage them to lobby/fork for adding that feature to Carthage. I don't think CocoaPods users should miss out on using subspecs just because they don't exist in Carthage.

@bobgodwinx
Copy link
Member

@ashfurrow

If Carthage users feel they're missing out, we should encourage them to lobby/fork for adding that feature to Carthage. I don't think CocoaPods users should miss out on using subspecs just because they don't exist in Carthage.

Look I am a fan of CocoaPods but I would admit that this would be unfair towards Carthage users. The best solution is to use another repository in order to satisfy both type of users.
Having said that, I think Carthage users will always find a way to solve this. It looks exactly the same thing repeating it's self in this issue

@freak4pc
Copy link
Member Author

A separate repo is the worst of all options IMO.
It doesn’t solve anything two separate podspecs in the same repo don’t solve.

@bobgodwinx
Copy link
Member

@freak4pc

A separate repo is the worst of all options IMO.

How really does a new repo focus on providing the above mentioned solution makes it a worse solution? Am I missing something?

@ashfurrow
Copy link
Member

I have to disagree about being unfair towards Carthage users – supporting the full features of CocoaPods that don't exist in Carthage doesn't negatively impact Carthage users, it's a purely additive change that helps CocoaPods users.

While multiple repos are a good solution to this problem theoretically, they introduce a tonne of unexpected overhead for maintainers. I think we should weigh the costs of having to keep multiple repos in sync with the value of separating the code. I worry that if we make the project more difficult to contribute to, we will see fewer new/returning contributors. Am I making sense?

@bobgodwinx
Copy link
Member

@ashfurrow I get what you are saying. All I am trying to communicate is that there is no perfect solution for Carthage users. I've tried locally all possible scenarios, even with Submodules and it didn't fix it. They have to use Carfile.ignore that's the only viable solution for them. Secondly it's difficult to change a dependency manager when you're in a team. This was my thoughts and that's why I was considering the poor Bob using Carthage.

@freak4pc
Copy link
Member Author

freak4pc commented Dec 13, 2017

We're considering Carthage users anyways - But do try and understand, splitting to two separate repo just to facilitate to the disadvantages of a single Dependency Manager isn't a good enough reason IMO.

In any other case consolidating feels like the best though.

First and foremost as @ashfurrow said

if we make the project more difficult to contribute to, we will see fewer new/returning contributors

It has already been very hard to get people involved. I think a singular project that would be the beef of extensions for the main RxSwift and RxCocoa would be a) a good starting point for developer looking for extras b) a great place for people to get involved with additions

Besides, it is much easier to schedule releases, organize documentation and have shared common code (even such that is not related to the code-base itself eg. danger, swiftlint and various standards).

I just really do not see a good reason to split to two repositories. It would just be much much more work to maintain and harder to reason for IMO.

@ashfurrow
Copy link
Member

@bobgodwinx I applaud your consideration for your peers, that's awesome. On Moya, we try to make sure that if something does break for Carthage users, we automate a check to prevent it from happening again:

https://github.com/Moya/Moya/blob/268b4c66b14efb7ee2aafe4cf32d435c52e9f43d/Dangerfile#L21-L28

I think applying similar logic, especially through Peril, would be prudent for this whole org.

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

6 participants