Skip to content

docs: rewrite Observable examples with pipable operators - #31074

Closed
lacolaco wants to merge 1 commit into
angular:masterfrom
lacolaco:aio-comparing-observables-pipe
Closed

docs: rewrite Observable examples with pipable operators#31074
lacolaco wants to merge 1 commit into
angular:masterfrom
lacolaco:aio-comparing-observables-pipe

Conversation

@lacolaco

@lacolaco lacolaco commented Jun 16, 2019

Copy link
Copy Markdown
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #31024

Observable examples are outdated.

What is the new behavior?

Using up-to-date pipeable operators

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@lacolaco
lacolaco requested a review from a team June 16, 2019 01:53
lacolaco
lacolaco commented Jun 16, 2019
View reviewed changes
Comment thread aio/content/guide/comparing-observables.md Outdated

@lacolaco lacolaco Jun 16, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

concat operator is deprecated. Currently, static concat creator function is only recommended.

@brandonroberts brandonroberts added comp: docs effort1: hours target: patch This PR is targeted for the next patch release risk: low state: community Someone from the Angular community is working on this issue or submitted this PR refactoring Issue that involves refactoring or code-cleanup labels Jun 17, 2019
@ngbot ngbot Bot added this to the Backlog milestone Jun 17, 2019
@brandonroberts brandonroberts added docsarea: observables and removed target: patch This PR is targeted for the next patch release comp: docs effort1: hours risk: low state: community Someone from the Angular community is working on this issue or submitted this PR refactoring Issue that involves refactoring or code-cleanup labels Jun 17, 2019
@brandonroberts brandonroberts self-assigned this Jun 17, 2019
@ngbot ngbot Bot modified the milestone: Backlog Jun 17, 2019
@mary-poppins

mary-poppins commented Jun 17, 2019

Copy link
Copy Markdown

You can preview 284056b at https://pr31074-284056b.ngbuilds.io/.

@brandonroberts brandonroberts added comp: docs effort1: hours target: patch This PR is targeted for the next patch release risk: low state: community Someone from the Angular community is working on this issue or submitted this PR refactoring Issue that involves refactoring or code-cleanup labels Jun 17, 2019
@ngbot ngbot Bot added this to the Backlog milestone Jun 17, 2019
brandonroberts
brandonroberts reviewed Jun 17, 2019
View reviewed changes
Comment thread aio/content/guide/comparing-observables.md Outdated

@brandonroberts brandonroberts Jun 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Separate issue, but this seems odd that we aren't using reduce here.

@lacolaco lacolaco Jun 17, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

indeed. ➞1➞3➞6➞11➞18 is not corresponding to array-side result (18).

@brandonroberts brandonroberts Jun 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will you go ahead and change it to use reduce instead?

@lacolaco lacolaco Jun 18, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. give me a day.

@lacolaco lacolaco Jun 18, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@brandonroberts I think it is not bad to include the change in this PR. should I separate it?

@brandonroberts brandonroberts Jun 18, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can include it in this one

@mary-poppins

mary-poppins commented Jun 18, 2019

Copy link
Copy Markdown

You can preview a21eb7e at https://pr31074-a21eb7e.ngbuilds.io/.

@brandonroberts

brandonroberts commented Jun 18, 2019

Copy link
Copy Markdown
Contributor

@lacolaco you will to rebase on latest master to fix the CI failure, and squash down to a single commit.

@lacolaco
lacolaco force-pushed the aio-comparing-observables-pipe branch from a21eb7e to 70ee7da Compare June 18, 2019 14:06
@mary-poppins

mary-poppins commented Jun 18, 2019

Copy link
Copy Markdown

You can preview 70ee7da at https://pr31074-70ee7da.ngbuilds.io/.

@brandonroberts brandonroberts added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jun 18, 2019
@ngbot ngbot Bot added the action: merge The PR is ready for merge by the caretaker label Jun 18, 2019
@ngbot

ngbot Bot commented Jun 18, 2019

Copy link
Copy Markdown

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci-codefresh-bazel" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@brandonroberts

brandonroberts commented Jun 18, 2019

Copy link
Copy Markdown
Contributor

Caretaker note: I am using a global approval for docs. The Codefresh failure is unrelated

AndrewKushnir pushed a commit that referenced this pull request Jun 18, 2019
@lacolaco @AndrewKushnir
docs: rewrite Observable examples with pipable operators (#31074)
eb970b4 
PR Close #31074
@lacolaco
lacolaco deleted the aio-comparing-observables-pipe branch June 19, 2019 00:54
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 15, 2019

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@brandonroberts brandonroberts brandonroberts approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@brandonroberts brandonroberts

Labels

action: merge The PR is ready for merge by the caretaker cla: yes effort1: hours merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note refactoring Issue that involves refactoring or code-cleanup risk: low state: community Someone from the Angular community is working on this issue or submitted this PR target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

4 participants

@lacolaco @mary-poppins @brandonroberts @googlebot