Skip to content
This repository was archived by the owner on Oct 1, 2018. It is now read-only.

Conversation

@feloy
Copy link
Contributor

@feloy feloy commented Oct 28, 2017

Refs #116

@codecov-io
Copy link

codecov-io commented Oct 28, 2017

Codecov Report

Merging #139 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #139   +/-   ##
=======================================
  Coverage   88.46%   88.46%           
=======================================
  Files           7        7           
  Lines          78       78           
  Branches        7        7           
=======================================
  Hits           69       69           
  Misses          6        6           
  Partials        3        3

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0ae1f0...2943059. Read the comment docs.

Copy link
Collaborator

@btroncone btroncone 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 your contribution! Just a few comments. 👍

{
name: "Pivot data on the id field",
code: `
Rx.Observable.of<Obj>({id: 1, name: 'aze1'},
Copy link
Collaborator

Choose a reason for hiding this comment

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

This appears invalid, did you mean <Object>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, no, I wanted to indicate a specific object, named Obj, defined this way in TypeScript:

interface Obj {
id number;
name: string;
}

{
name: "Group objects by id and return as array",
code: `
Rx.Observable.of<Obj>({id: 1, name: 'aze1'},
Copy link
Collaborator

Choose a reason for hiding this comment

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

See other comment regarding Object

shortDescription: {
description: `
Group, according to a specified key, elements from items emitted by an Observable,
and emit these grouped items as GroupedObservables, one GroupedObservable per group.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there something we can link to here for GroupedObservable that provides a better explanation? Since it is mentioned several times in the explanation I think it would be easy to get lost within an understanding of this concept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I added some explanation about GroupedObservable.

@ladyleet
Copy link
Member

@feloy can you update your branch pls? Otherwise, thx for this awesome Pr

@feloy
Copy link
Contributor Author

feloy commented Nov 16, 2017

@ladyleet Hi, I think it's updated now.

@ladyleet ladyleet merged commit c73d8a2 into ReactiveX:master Nov 16, 2017
@ladyleet
Copy link
Member

Thx @feloy! Merged! Thx @ashwin-sureshkumar for peeping at this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants