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

Add ReactiveX deps, including JakeWharton's RxBinding and RxRelay #478

Closed
wants to merge 2 commits into from

Conversation

mgray88
Copy link
Contributor

@mgray88 mgray88 commented Dec 22, 2021

🚀 Description

Starting a draft PR to gather input on these dependencies, I will work on documentation in the meantime. There were a few things I wasn't sure about. Particularly the alias rules, I see some in txt files and some in the code, I wasn't sure what the recommended method was going forward. Also didn't really understand the removals and related generated things

Add ReactiveX dependencies:

  • RxJava, RxJava2, RxJava3
  • RxAndroid
  • RxKotlin

Add Rx libraries from Jake Wharton:

  • RxBinding3 and RxBinding4
  • RxRelay2 and RxRelay3

📄 Motivation and Context

These are widely used libraries, many could probably benefit

🧪 How Has This Been Tested?

Ran tests. They passed!

📦 Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

✅ Checklist

@brady-aiello
Copy link
Contributor

@mgray88
Copy link
Contributor Author

mgray88 commented Jan 4, 2022

Oops! Missed that requirement. Looks like there's an existing issue, I'll link it

I didn't really understand that autogenerated README entry. It's not really clear what I'm supposed to do with it?

Copy link
Member

@LouisCAD LouisCAD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! There are some concerns, so I added comments in this review for you 🙂

BTW, it might be easier for both of us to address these in a new PR, fresh from the current main branch, superseding this draft PR.

@brady-aiello It's a markdown file, but it's not a README 😄
Anyway, the act of solely adding dependency notations should never touch this file, so… 🙃

@mgray88
Copy link
Contributor Author

mgray88 commented Jan 10, 2022

@LouisCAD Closing to reopen in a new PR after rebase and refactor :)

@mgray88 mgray88 closed this Jan 10, 2022
@LouisCAD
Copy link
Member

I'd advise extracting a patch and apply select changes instead of rebasing, but maybe your plan will work just as well.

@LouisCAD
Copy link
Member

Hello Mike, if you still plan on getting these added, please take the time to open the new PR as you told, or tell me you'll not do it, so I or someone else know they can do it. :)

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.

None yet

3 participants