refactor(forms): match getError and hasError to get method signature by Toxicable · Pull Request #20211 · angular/angular

Toxicable · 2017-11-06T08:13:16Z

closes #19734

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Feature

What is the current behavior?

getError and hasError mismatch the api of get

Issue Number: #19734

What is the new behavior?

apis match

Does this PR introduce a breaking change?

[ ] Yes
[x] No

kara

Looks good, but can we also get a test for hasError, since we are changing it here?

Toxicable · 2018-02-07T07:29:03Z

@kara done

kara

LGTM

kara · 2018-02-07T23:13:23Z

@IgorMinar API review?

IgorMinar · 2018-02-08T06:45:19Z

Please update api docs for both methods.

Wasn't sure what to change here so I added a note on how the path can be used.
Let me know if you want something else

@Toxicable I took a look again, but wasn't able to find the docs that you added. Is the commit up to date?

@kara Uhhh, I guess not sorry, Ill double check this afternoon

@kara Added some detail on what the path is meant to be and how to use it

Looks good. I'll let @IgorMinar take one more look though, while it's presubmitting.

IgorMinar · 2018-02-08T06:47:19Z

Why number? Seems odd. And there are no tests as far as I can tell for using numerical arrays.

Seams valid if you're traversing through a form array - form arrya being index based.
Ill add some tests.

kara

LGTM. This last change fixed the internal failures (and don't worry about timing, you are still doing better than we are :) ).

kara · 2018-12-21T02:08:35Z

@IgorMinar I think it needs one more review from you since the public-API changed again.

IgorMinar

Thank you, @Toxicable!!!

IgorMinar

dang! I just noticed that this PR also needs API docs update. I asked @kara to send a follow up PR so that we don't need to block this, but in the future @Toxicable please do update the API docs.

I also noticed that you wrote a great justification of this change in the issue, but that justification is not available in the commit message, in the future it would be easier for us to review these kinds of changes if the explanation was in the commit message. thank you

IgorMinar

dang #2!

this commit should have been a feature or a fix since it changes the public api surface
otherwise the change won’t get highlighted in the changelog and nobody will know

Toxicable · 2018-12-22T00:31:28Z

@IgorMinar I can change it to a fix now if you like?

kara · 2018-12-22T00:32:59Z

@Toxicable If you can change it to a fix and add the justification to the commit message, that'd be awesome! We can get the PR in sooner. I can add docs in a follow-up so we don't have to go through the whole review process again.

Internally getError and hasError call the AbstractControl#get method which takes `path: Array<string | number> | string` as input, since there are different ways to traverse the AbstractControl tree. This change matches the method signitures of all methods that use this.

Toxicable · 2018-12-22T00:40:59Z

@kara done!

This commit adds docs for the changes made in angular#20211. Closes angular#19734.

This commit adds docs for the changes made in #20211. Closes #19734. PR Close #27861

angular-automatic-lock-bot · 2019-09-14T11:52:59Z

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.}

googlebot added the cla: yes label Nov 6, 2017

Toxicable force-pushed the forms-geterror branch from 160f091 to f7e0d30 Compare November 6, 2017 08:20

Toxicable changed the title ~~feat(forms): match getError has hasError to get's signature~~ feat(forms): match getError and hasError to get's signature Nov 6, 2017

Toxicable force-pushed the forms-geterror branch 2 times, most recently from dc32358 to e20db06 Compare November 6, 2017 09:01

vicb added action: review The PR is still awaiting reviews from at least one requested reviewer area: forms labels Nov 6, 2017

kara self-assigned this Nov 30, 2017

kara added the refactoring Issue that involves refactoring or code-cleanup label Nov 30, 2017

kara changed the title ~~feat(forms): match getError and hasError to get's signature~~ refactor(forms): match getError and hasError to get's signature Nov 30, 2017

kara changed the title ~~refactor(forms): match getError and hasError to get's signature~~ refactor(forms): match getError and hasError to get method signature Nov 30, 2017

Toxicable force-pushed the forms-geterror branch from e20db06 to 23eab6d Compare December 1, 2017 19:15

Toxicable force-pushed the forms-geterror branch from 23eab6d to e045728 Compare December 9, 2017 20:52

kara reviewed Feb 6, 2018

View reviewed changes

kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 6, 2018

kara assigned Toxicable and unassigned kara Feb 6, 2018

Toxicable force-pushed the forms-geterror branch 3 times, most recently from 5a53e61 to 395ff1b Compare February 7, 2018 07:13

kara approved these changes Feb 7, 2018

View reviewed changes

kara added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 7, 2018

kara requested a review from IgorMinar February 7, 2018 23:13

kara assigned IgorMinar and unassigned Toxicable Feb 7, 2018

IgorMinar suggested changes Feb 8, 2018

View reviewed changes

kara approved these changes Dec 21, 2018

View reviewed changes

kara assigned IgorMinar and unassigned Toxicable Dec 21, 2018

kara added target: major This PR is targeted for the next major release and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 21, 2018

IgorMinar approved these changes Dec 22, 2018

View reviewed changes

IgorMinar reviewed Dec 22, 2018

View reviewed changes

kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 22, 2018

Toxicable force-pushed the forms-geterror branch from bcde6d1 to 01b8874 Compare December 22, 2018 00:39

kara removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 22, 2018

kara unassigned IgorMinar Dec 22, 2018

kara added the action: merge The PR is ready for merge by the caretaker label Dec 22, 2018

kara mentioned this pull request Dec 28, 2018

docs(forms): update desc for hasError and getError #27861

Closed

kara added a commit to kara/angular that referenced this pull request Dec 28, 2018

docs(forms): update desc for hasError and getError

11556b2

This commit adds docs for the changes made in angular#20211. Closes angular#19734.

benlesh pushed a commit that referenced this pull request Jan 3, 2019

docs(forms): update desc for hasError and getError (#27861)

e268a05

This commit adds docs for the changes made in #20211. Closes #19734. PR Close #27861

benlesh closed this in 1b0b36d Jan 3, 2019

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 14, 2019

Conversation

Toxicable commented Nov 6, 2017

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Uh oh!

kara left a comment

Choose a reason for hiding this comment

Uh oh!

Toxicable commented Feb 7, 2018

Uh oh!

kara left a comment

Choose a reason for hiding this comment

Uh oh!

kara commented Feb 7, 2018

Uh oh!

IgorMinar Feb 8, 2018

Choose a reason for hiding this comment

Uh oh!

Toxicable Feb 8, 2018

Choose a reason for hiding this comment

Uh oh!

kara Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

Toxicable Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

Toxicable Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

kara Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

IgorMinar Feb 8, 2018

Choose a reason for hiding this comment

Uh oh!

Toxicable Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kara left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kara commented Dec 21, 2018

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

Toxicable commented Dec 22, 2018

Uh oh!

kara commented Dec 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Toxicable commented Dec 22, 2018

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Toxicable Feb 8, 2018 •

edited

Loading

kara left a comment •

edited

Loading

kara commented Dec 22, 2018 •

edited

Loading