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): update email validator to inherit abstractValidator #44545

Closed

Conversation

iRealNirmal
Copy link
Contributor

Modified email validator to inherit abstractValidator.

For every validato type different PR will be raised as discussed in #42378.

Closes #42267

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: #42378

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@pullapprove pullapprove bot requested a review from dylhunn December 22, 2021 05:18
@iRealNirmal iRealNirmal marked this pull request as draft December 22, 2021 05:18
@iRealNirmal iRealNirmal force-pushed the required-email-abstractvalidator branch from 8a10246 to 678e5c8 Compare December 22, 2021 16:10
@iRealNirmal
Copy link
Contributor Author

@dylhunn @AndrewKushnir can you help me rerun lint job, when am running in my local am not seeing in any changes let me also know if you see any other difference if you can run in your local.

Help here will be appreciated, thanks.

@ngbot ngbot bot added this to the Backlog milestone Jan 4, 2022
@dylhunn
Copy link
Contributor

dylhunn commented Jan 4, 2022

@iRealNirmal You should be able to run the formatting command, then push again and it will rerun automatically. Let me know if that doesn't work.

@dylhunn dylhunn removed their request for review January 4, 2022 20:37
@dylhunn
Copy link
Contributor

dylhunn commented Jan 4, 2022

BTW do you want a review on this? pullapprove added me and I'm not sure why. Remove draft status when you're ready :)

@AndrewKushnir
Copy link
Contributor

FYI, this PR is blocked by #44500, which we'd need to land first.

@iRealNirmal
Copy link
Contributor Author

@iRealNirmal You should be able to run the formatting command, then push again and it will rerun automatically. Let me know if that doesn't work.

@dylhunn I am running in my local but unfortunately it's not showing any changes at all.
If that is something you can help that will be great and also if you have any other comments in PR feel free to do it.

@iRealNirmal iRealNirmal marked this pull request as ready for review January 6, 2022 12:11
@pullapprove pullapprove bot requested a review from dylhunn January 6, 2022 12:12
@iRealNirmal iRealNirmal force-pushed the required-email-abstractvalidator branch 3 times, most recently from 2adc435 to b007287 Compare January 6, 2022 13:55
@iRealNirmal
Copy link
Contributor Author

iRealNirmal commented Jan 6, 2022

after rebase things are failing so marking back to draft mode, but still help appreciated for lint because it's still there

@iRealNirmal iRealNirmal marked this pull request as draft January 6, 2022 14:10
@iRealNirmal
Copy link
Contributor Author

@AndrewKushnir thanks for your input it helped to speed up task, but am still facing issue for lint at

override normalizeInput = (input: unknown): boolean =>
      input === '' || input === true || input === 'true'

earlier it was giving error for Unnecessary semicolon I removed but it's giving various errors for this line. I can try various other way but you might have already faced this issue and would have created some defined guide line so that's why I am waiting for your response before doing anything else.

iRealNirmal and others added 2 commits January 10, 2022 08:57
Modified email validator to inherit abstractValidator.

For every validato type different PR will be raised as discussed in angular#42378.

Closes angular#42267
@AndrewKushnir AndrewKushnir force-pushed the required-email-abstractvalidator branch from 08fdebf to 7571264 Compare January 10, 2022 17:18
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jan 10, 2022

@iRealNirmal I've pushed a fixup commit that adds a semicolon. I've also ran lint command and rebased this PR on top of the most recent version of the main branch.

@iRealNirmal iRealNirmal marked this pull request as ready for review January 11, 2022 01:25
@iRealNirmal
Copy link
Contributor Author

Thanks @AndrewKushnir , it's strange why running lint command in my local was not correcting it, it neither added semicolon nor brackets. Did it worked directly for you ? If yes then I will have to do something for it in my local.

@iRealNirmal
Copy link
Contributor Author

Ohh it's something tslint error and you disabled next line, thanks for taking care of it.

@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 11, 2022
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@iRealNirmal the changes look good, thanks 👍

I've started tests in Google's codebase (internal-only link) and will let you know how it goes.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@pullapprove pullapprove bot requested a review from alxhub January 11, 2022 16:49
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

LGTM 🍪

reviewed-for: public-api

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Jan 11, 2022
@AndrewKushnir
Copy link
Contributor

@iRealNirmal FYI, some code in Google's codebase require a minor cleanup, but everything is now ready for merge. I'm adding this PR to the merge queue.

Thanks again for all your efforts to unify the code of the validators! 👍 That really helps to make it more maintainable and more compact (so validators contribute less to bundle sizes).

@atscott
Copy link
Contributor

atscott commented Jan 12, 2022

This PR was merged into the repository by commit c7b210d.

@atscott atscott closed this in c7b210d Jan 12, 2022
atscott pushed a commit that referenced this pull request Jan 12, 2022
…44545)

Modified email validator to inherit abstractValidator.

For every validato type different PR will be raised as discussed in #42378.

Closes #42267

PR Close #44545
amitbeck pushed a commit to amitbeck/angular that referenced this pull request Jan 13, 2022
…ngular#44545)

Modified email validator to inherit abstractValidator.

For every validato type different PR will be raised as discussed in angular#42378.

Closes angular#42267

PR Close angular#44545
@angular-automatic-lock-bot
Copy link

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 Feb 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: forms forms: validators target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression introduced by new min max validators
6 participants