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

docs(operators): add fromPromise documentation #229

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

niklas-wortmann
Copy link
Member

closes: #79

@codecov-io
Copy link

codecov-io commented Jan 4, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #229   +/-   ##
=======================================
  Coverage   90.49%   90.49%           
=======================================
  Files         115      115           
  Lines         442      442           
  Branches       10       10           
=======================================
  Hits          400      400           
  Misses         40       40           
  Partials        2        2
Impacted Files Coverage Δ
src/operator-docs/creation/fromPromise.ts 100% <ø> (ø) ⬆️

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 192a02b...3266e74. 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.

Small typo in description, other than that looks great! 👍

type: 'Scheduler',
attribute: 'optional',
description:
'An optional IScheduler to use for scheduling the delivery of the resolved value (or the rejection).s'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think there's a small typo at end of sentence (or the rejection).s

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah, you're right. Thanks for the hint. Will fix it this afternoon.

fix typo in the description of fromPromise operator
@ashwin-sureshkumar
Copy link
Collaborator

@btroncone - Could you please re-review ?

@niklas-wortmann
Copy link
Member Author

@ashwin-sureshkumar any idea why travis is broken? there is no log or something like that

@ashwin-sureshkumar
Copy link
Collaborator

Its one of the dependencies @jwo719.

This PR fixes it #236

Can you please review that PR, we have to have two approvals before we merge in. Then you can rebase with master and should be good.

@ladyleet
Copy link
Member

thx @jwo719 @ashwin-sureshkumar @btroncone for all your hard work!

name: 'Convert the Promise returned by Fetch to an Observable',
code: `
const result = Rx.Observable.fromPromise(fetch('http://myserver.com/'));
result.subscribe(x => console.log(x), e => console.error(e));`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

please change these to es6 imports, there is an incoming PR from @btroncone converting the existing examples.

@niklas-wortmann
Copy link
Member Author

@btroncone @ashwin-sureshkumar last commit should fix both requested changes. Please take a look at it and let me know if there is something else I can fix.

code: `
import {fromPromise} from 'rxjs/observable/fromPromise';

const result = fromPromise(fetch('http://myserver.com/'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we use an actual API example here github or any other?

Copy link
Member Author

@niklas-wortmann niklas-wortmann Feb 5, 2018

Choose a reason for hiding this comment

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

this one should be fixed

import {fromPromise} from 'rxjs/observable/fromPromise';

const result = fromPromise(fetch('http://myserver.com/'));
result.subscribe(x => console.log(x), e => console.error(e));`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add expected output also.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this would be rather confusing in this case. Because the operator is more or less like a cast and doesn't manipulate the output. What do you think? Of course I can add an output, but I think it won't add much value in this case.

@niklas-wortmann
Copy link
Member Author

@btroncone, @ashwin-sureshkumar, @sumitarora could you take another look at this pr to get this done?

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

Successfully merging this pull request may close these issues.

Add docs for operator : creation - fromPromise
6 participants