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

feat: isAlphaNumeric #43

Merged
merged 2 commits into from
Oct 27, 2019
Merged

feat: isAlphaNumeric #43

merged 2 commits into from
Oct 27, 2019

Conversation

floookay
Copy link
Contributor

Added the isAlphaNumeric feature to check for strings that consist of alphabetical and/or numerical characters.
Aliases are isAlphaNumeric, isAlphanumeric and isAlphaDigit

Closes #20

@floookay
Copy link
Contributor Author

@vorillaz Hi, can you help me understand why the test failed? I'm not sure if I understand the problem. I already did a rebase.

@vorillaz
Copy link
Contributor

Hey @floookay you can see the failing spec over here circleci.com/gh/plexis-js/plexis/122

Every time you are adding a new dependency to the main plexis package gets expanding. The main contains all the subpackages available.
So there is a single test inside plexis asserting the output methods. (

expect(Object.keys(plexis)).toMatchSnapshot();
)

This one is based on snapshot testing. Since you have added a new method into the exported ones you shall update the snapshot (https://github.com/plexis-js/plexis/blob/master/packages/plexis/test/__snapshots__/index.js.snap)

You can pipe any Jest parameters directly to the NPM script. So you can run yarn test -u and BOOM the snapshot gets updated.

More about snapshot testing can be found here
The presence of this test ensures that we are cross-checking each method added to the main package.
Does this make sense to you?

@brandon-m-skinner
Copy link
Contributor

Hey @floookay,

Make sure you run yarn and possibly yarn bootstrap to make sure everything is where it needs to be. All tests should then pass locally with yarn test. Once that's the case, update the snapshot with yarn test -u.

@vorillaz should probably expand the README or add some sort of instructions with common problems. This issue in particular keeps coming up.

Brandon

@vorillaz
Copy link
Contributor

Thanks for the suggestion @brandon-m-skinner that's an awesome idea.
Is there any chance you could expand the docs via a PR with what you have in mind since I am a bit busy reviewing PRs and creating new issues?

@brandon-m-skinner
Copy link
Contributor

Sure thing @vorillaz.

@floookay
Copy link
Contributor Author

@vorillaz Thank you so much for the detailed explanation! I see were I messed up earlier. Also, running yarn bootstrap did the trick. I was wondering why the --updateSnapshot command didn't produce any changes prior. Thanks @brandon-m-skinner!

@vorillaz
Copy link
Contributor

Hey @floookay , can you please rebase your branch and we are good to go.

@codecov
Copy link

codecov bot commented Oct 27, 2019

Codecov Report

Merging #43 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #43   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          25     26    +1     
  Lines         129    130    +1     
=====================================
+ Hits          129    130    +1
Impacted Files Coverage Δ
packages/isAlphaNumeric/src/index.js 100% <100%> (ø)

@floookay
Copy link
Contributor Author

floookay commented Oct 27, 2019

@vorillaz The branch is rebased.

@vorillaz
Copy link
Contributor

@floookay Can you please do another rebase since I released a version? Apologies for that :(

@floookay
Copy link
Contributor Author

@vorillaz No worries! I rebased the branch.

@vorillaz vorillaz merged commit 6e51bfb into plexis-js:master Oct 27, 2019
@vorillaz
Copy link
Contributor

@floookay Thanks so much :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: isAlphaDigit
3 participants