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

Validator Class Added with Builder Pattern #1749

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

Conversation

SankalpaFernando
Copy link

@SankalpaFernando SankalpaFernando commented Oct 2, 2021

Validator Class Added (For Function Chaining)

Added a Validator Class that using the builder pattern to validate the data using chained validation functions

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

@codecov
Copy link

codecov bot commented Oct 2, 2021

Codecov Report

Merging #1749 (7ad9d0d) into master (c899b31) will decrease coverage by 3.28%.
The diff coverage is 58.68%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master    #1749      +/-   ##
===========================================
- Coverage   100.00%   96.71%   -3.29%     
===========================================
  Files          102      104       +2     
  Lines         2015     2103      +88     
  Branches       454      470      +16     
===========================================
+ Hits          2015     2034      +19     
- Misses           0       69      +69     
Impacted Files Coverage Δ
src/lib/builder.js 2.81% <2.81%> (ø)
src/index.js 100.00% <100.00%> (ø)
src/lib/index.js 100.00% <100.00%> (ø)
src/lib/isEmail.js 100.00% <0.00%> (ø)
src/lib/isIdentityCard.js 100.00% <0.00%> (ø)

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 c899b31...7ad9d0d. Read the comment docs.

Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

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

Hello @SankalpaFernando
Thank you for your PR! Is it a work in progress?

@SankalpaFernando
Copy link
Author

SankalpaFernando commented Oct 3, 2021

@tux-tn The validators need to be added. I'll add them in a future PR

@fedeci
Copy link
Contributor

fedeci commented Oct 3, 2021

We still need tests here.

@SankalpaFernando
Copy link
Author

Created a new file called index.js in the ./lib directory and put all the validator exporting in that file. Meantime, a separate test file was created to test the validator class.

@tux-tn
Copy link
Member

tux-tn commented Oct 3, 2021

I'm not sure we want to move or add a builder/chaining pattern to the library.
Since it's a big change,I suggest we wait for the community feedback about this feature.
cc @profnandaa @ezkemboi

@tux-tn tux-tn added blocked For PRs that are blocked due to pending discussions, etc. 🍿 discussion needs-tests labels Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked For PRs that are blocked due to pending discussions, etc. 🍿 discussion needs-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants