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.

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

refactor(forms): match getError and hasError to get method signature #20211

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@Toxicable
Copy link
Contributor

Toxicable commented Nov 6, 2017

closes #19734

PR Checklist

Please check if your PR fulfills the following requirements:

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

@googlebot googlebot added the cla: yes label Nov 6, 2017

@Toxicable Toxicable force-pushed the Toxicable:forms-geterror branch from 160f091 to f7e0d30 Nov 6, 2017

@Toxicable 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 Toxicable force-pushed the Toxicable:forms-geterror branch 2 times, most recently from dc32358 to e20db06 Nov 6, 2017

@kara kara self-assigned this Nov 30, 2017

@kara kara added the type: refactor label Nov 30, 2017

@kara 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 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 Toxicable force-pushed the Toxicable:forms-geterror branch from e20db06 to 23eab6d Dec 1, 2017

@Toxicable Toxicable force-pushed the Toxicable:forms-geterror branch from 23eab6d to e045728 Dec 9, 2017

@kara
Copy link
Contributor

kara left a comment

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

@kara kara assigned Toxicable and unassigned kara Feb 6, 2018

@Toxicable Toxicable force-pushed the Toxicable:forms-geterror branch 3 times, most recently from 5a53e61 to 395ff1b Feb 7, 2018

@Toxicable

This comment has been minimized.

Copy link
Contributor Author

Toxicable commented Feb 7, 2018

@kara done

@kara

kara approved these changes Feb 7, 2018

Copy link
Contributor

kara left a comment

LGTM

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Feb 7, 2018

@IgorMinar API review?

@kara kara requested a review from IgorMinar Feb 7, 2018

@kara kara assigned IgorMinar and unassigned Toxicable Feb 7, 2018

@@ -537,7 +537,9 @@ export abstract class AbstractControl {
*
* If no path is given, it checks for the error on the present control.

This comment has been minimized.

@IgorMinar

IgorMinar Feb 8, 2018

Member

Please update api docs for both methods.

This comment has been minimized.

@Toxicable

Toxicable Feb 8, 2018

Author Contributor

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

This comment has been minimized.

@kara

kara Feb 20, 2018

Contributor

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

This comment has been minimized.

@Toxicable

Toxicable Feb 21, 2018

Author Contributor

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

This comment has been minimized.

@Toxicable

Toxicable Feb 21, 2018

Author Contributor

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

This comment has been minimized.

@kara

kara Feb 22, 2018

Contributor

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

@@ -537,7 +537,9 @@ export abstract class AbstractControl {
*
* If no path is given, it checks for the error on the present control.
*/
hasError(errorCode: string, path?: string[]): boolean { return !!this.getError(errorCode, path); }
hasError(errorCode: string, path?: Array<string|number>|string): boolean {

This comment has been minimized.

@IgorMinar

IgorMinar Feb 8, 2018

Member

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

This comment has been minimized.

@Toxicable

Toxicable Feb 8, 2018

Author Contributor

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

@Toxicable

This comment has been minimized.

Copy link
Contributor Author

Toxicable commented Dec 19, 2018

@kara Hey, sorry the turn around from my side has been so slow.
updated those signatures aswell and re-rebased, let me know if I missed anything else

@Toxicable Toxicable force-pushed the Toxicable:forms-geterror branch from 1cc91a2 to bcde6d1 Dec 19, 2018

@kara

kara approved these changes Dec 21, 2018

Copy link
Contributor

kara left a comment

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

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Dec 21, 2018

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

@IgorMinar
Copy link
Member

IgorMinar left a comment

Thank you, @Toxicable!!!

@IgorMinar
Copy link
Member

IgorMinar left a comment

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
Copy link
Member

IgorMinar left a comment

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

This comment has been minimized.

Copy link
Contributor Author

Toxicable commented Dec 22, 2018

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

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Dec 22, 2018

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

fix(forms): match getError and hasError to get method signature
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 Toxicable force-pushed the Toxicable:forms-geterror branch from bcde6d1 to 01b8874 Dec 22, 2018

@Toxicable

This comment has been minimized.

Copy link
Contributor Author

Toxicable commented Dec 22, 2018

@kara done!

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

docs(forms): update desc for hasError and getError
This commit adds docs for the changes made in angular#20211.

Closes angular#19734.

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

docs(forms): update desc for hasError and getError
This commit adds docs for the changes made in angular#20211.

Closes angular#19734.

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

docs(forms): update desc for hasError and getError (#27861)
This commit adds docs for the changes made in #20211.

Closes #19734.

PR Close #27861

@benlesh benlesh closed this in 1b0b36d Jan 3, 2019

devversion added a commit to devversion/angular that referenced this pull request Jan 9, 2019

devversion added a commit to devversion/angular that referenced this pull request Jan 9, 2019

fix(forms): match getError and hasError to get method signature (angu…
…lar#20211)

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.

PR Close angular#20211

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

fix(forms): match getError and hasError to get method signature (angu…
…lar#20211)

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.

PR Close angular#20211
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.