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.

Sign up for GitHub

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

Jump to bottom

refactor(AjaxObservable): remove needles type param T from AjaxObservable.getJSON() #2069

Merged
merged 1 commit into from
Nov 4, 2016
Merged

refactor(AjaxObservable): remove needles type param T from AjaxObservable.getJSON() #2069

merged 1 commit into from
Nov 4, 2016

Conversation

tetsuharuohzeki
Copy link
Contributor

Description:

BREAKING CHANGE:

  • Remove the unused type parameter T
    • By this change, the user does not have to specify needless type parameter.

Related issue (if exists):

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.217% when pulling 6a63f7b on saneyuki:unused-type-param-of-AjaxObservable into 260d335 on ReactiveX:master.

@jayphelps
Copy link
Member

Minor change: "BREAKING CHANGE" messages in commits need to be fully descriptive on their own, independent of the first part of the commit message because when the CHANGELOG is generated, they live in their own section.

e.g. aa30af2 generates to https://github.com/ReactiveX/rxjs/blob/master/CHANGELOG.md#breaking-changes-1

So in this case, something like

BREAKING CHANGE: Observable.ajax.getJSON() now only supports a single type parameter, getJSON<R>(url: string, headers?: Object): Observable<R>. The extra type parameter it accepted previously was superfluous.

Double check all your other PRs have good BREAKING CHANGE messages too please. Also--related refactors like these should prolly have been in a single PR to ease in the discussion and tracking. #2072 #2071 #2070 #2068

@tetsuharuohzeki
Copy link
Contributor Author

Minor change: "BREAKING CHANGE" messages in commits need to be fully descriptive on their own, independent of the first part of the commit message because when the CHANGELOG is generated, they live in their own section.

Okay, I'll update.

related refactors like these should prolly have been in a single PR to ease in the discussion and tracking. #2072 #2071 #2070 #2068

They are originated from the same issues, but I thought they still are different issue. cleaning up codes or changing signature which we may need some decisions.

@tetsuharuohzeki tetsuharuohzeki force-pushed the unused-type-param-of-AjaxObservable branch from 6a63f7b to af37b13 Compare October 25, 2016 19:12
@tetsuharuohzeki
Copy link
Contributor Author

updated the commit message.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.217% when pulling af37b13 on saneyuki:unused-type-param-of-AjaxObservable into 260d335 on ReactiveX:master.

benlesh
benlesh requested changes Oct 26, 2016
View reviewed changes
src/observable/dom/AjaxObservable.ts
@@ -67,7 +67,7 @@ export interface AjaxCreationMethod {
post(url: string, body?: any, headers?: Object): Observable<AjaxResponse>;
put(url: string, body?: any, headers?: Object): Observable<AjaxResponse>;
delete(url: string, headers?: Object): Observable<AjaxResponse>;
getJSON<T, R>(url: string, headers?: Object): Observable<R>;
getJSON<R>(url: string, headers?: Object): Observable<R>;
Copy link
Member

Choose a reason for hiding this comment

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

should probably just be T... that's what we do everywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Blesh okay. I fixed in 501bac4

@tetsuharuohzeki tetsuharuohzeki force-pushed the unused-type-param-of-AjaxObservable branch from af37b13 to 501bac4 Compare October 26, 2016 02:51
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.217% when pulling 501bac4 on saneyuki:unused-type-param-of-AjaxObservable into 0271fab on ReactiveX:master.

@tetsuharuohzeki tetsuharuohzeki force-pushed the unused-type-param-of-AjaxObservable branch from 501bac4 to d864c9b Compare October 26, 2016 04:04
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.217% when pulling d864c9b on saneyuki:unused-type-param-of-AjaxObservable into 0271fab on ReactiveX:master.

@tetsuharuohzeki tetsuharuohzeki force-pushed the unused-type-param-of-AjaxObservable branch from ac68378 to 6c221bc Compare October 27, 2016 03:02
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.217% when pulling 6c221bc on saneyuki:unused-type-param-of-AjaxObservable into 89612b2 on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.217% when pulling 6c221bc on saneyuki:unused-type-param-of-AjaxObservable into 89612b2 on ReactiveX:master.

@tetsuharuohzeki @benlesh
fix(AjaxObservable): remove needless type param R from AjaxObservable…
d72d3c8 
….getJSON()

BREAKING CHANGE: Observable.ajax.getJSON() now only supports a single type parameter,
`getJSON<T>(url: string, headers?: Object): Observable<T>`.
The extra type parameter it accepted previously was superfluous.
@benlesh benlesh force-pushed the unused-type-param-of-AjaxObservable branch from 6c221bc to d72d3c8 Compare November 4, 2016 17:23
@benlesh benlesh merged commit 0c3d4a4 into ReactiveX:master Nov 4, 2016
@coveralls
Copy link

coveralls commented Nov 4, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 97.217% when pulling 501bac4 on saneyuki:unused-type-param-of-AjaxObservable into 0271fab on ReactiveX:master.

@tetsuharuohzeki tetsuharuohzeki deleted the unused-type-param-of-AjaxObservable branch November 4, 2016 19:15
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benlesh benlesh benlesh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tetsuharuohzeki @coveralls @jayphelps @benlesh