-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Modify misspellings #1634
Modify misspellings #1634
Conversation
Nice fixes @fuyoufang ! Do you mind adding a CHANGELOG entry ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fuyoufang ,
thanks for fixes, have some comments.
|
||
disposeEverything.disposable = cancelSchedule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a bug. let cancelSchedule = SingleAssignmentDisposable()
is used to ensure correct order of assignments to serial disposable in case scheduler performs synchronous execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you.
I resubmitted, but I'm not sure if it's correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, minor nit
CHANGELOG.md
Outdated
@@ -21,6 +21,8 @@ All notable changes to this project will be documented in this file. | |||
#### Anomalies | |||
|
|||
* Fixes ambiguity issue with `Single.do(onNext:onError:onSubscribe:onSubscribed:onDispose:)` and `Single.do(onSuccess:onError:onSubscribe:onSubscribed:onDispose:)`. | |||
* Fixes misspelled `immediatelly` to `immediately`, `Programatic` to `Programmatic`. | |||
* Adds missing `scheduler` parameter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make these two into one?
- Fixes various spelling mistakes and missing parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think you, I resubmitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @fuyoufang
Please limit this PR to misspelling checks and don't change any of the existing code :)
If you want to discuss code changes its better to do so in a separate PR.
I don't understand completely why you're modifying pieces of the code.
Thanks!
@@ -358,7 +358,7 @@ extension SharedSequence { | |||
} | |||
|
|||
/** | |||
Concatenates all observable sequences in the given sequence, as long as the previous observable sequence terminated successfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sequence == Collection, plus the type is SharedSequence
. I don't think this change is required.
Sorry, I didn't know enough about PR, so I resubmitted for other reason. Now I reset the commit, but I don't know if I should do this. Now, I know how to create a new PR. Thank you for your guidance. |
Hi @fuyoufang , It's ok to reset commits in PRs since nobody is based of them. |
Think you. |
Ok, I'll merge this into a temporary branch, and then rebase it to |
Modify the misspelling of
immediately
andProgrammatic