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.

Sign up for GitHub

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

Jump to bottom

[1673] Ensure mix_case returns at least one lower and one upper case … #1694

Merged
merged 1 commit into from
Aug 27, 2019
Merged

[1673] Ensure mix_case returns at least one lower and one upper case … #1694

merged 1 commit into from
Aug 27, 2019

Conversation

bpleslie
Copy link
Member

@bpleslie bpleslie commented Aug 19, 2019 •
edited
Loading

…letter

Resolves Issue #1673

Description:

In order to ensure that at least one lowercase and one uppercase letter was returned when passing mix_case: true to Internet.password, I did the following:

  • Allow Alphanumeric.alphanumeric to accept min_alpha and min_numeric params. (Note: min_numeric is not used, but seemed equally helpful for certain situations. I'm happy to remove it if desired.)
  • Implemented min_alpha in Internet.password altered the mix_case condition to ensure we always return at least one uppercase and one lowercase letter if mix_case is true.

@bpleslie bpleslie force-pushed the issue-1673-password-mix-case branch 3 times, most recently from 6a329d6 to db77d36 Compare August 19, 2019 22:15
Zeragamba
Zeragamba suggested changes Aug 20, 2019
View reviewed changes
Copy link
Contributor

@Zeragamba Zeragamba left a comment

Choose a reason for hiding this comment

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

Tests look good to me, and adding both min_alpha and min_numaric makes sense to me.

lib/faker/default/alphanumeric.rb Outdated Show resolved Hide resolved
lib/faker/default/alphanumeric.rb Outdated Show resolved Hide resolved
@Zeragamba
Copy link
Contributor

Oh, and docs need to be updated.

If possible it would be nice if you could include Yard style docs since that's in the pipeline, but not required.

@bpleslie bpleslie force-pushed the issue-1673-password-mix-case branch from db77d36 to cac7ab8 Compare August 20, 2019 02:37
Zeragamba
Zeragamba suggested changes Aug 20, 2019
View reviewed changes
lib/faker/default/alphanumeric.rb Outdated Show resolved Hide resolved
lib/faker/default/alphanumeric.rb Outdated Show resolved Hide resolved
lib/faker/default/alphanumeric.rb Outdated Show resolved Hide resolved
@bpleslie bpleslie force-pushed the issue-1673-password-mix-case branch from cac7ab8 to 1b9d99c Compare August 20, 2019 17:40
@bpleslie
Copy link
Member Author

Working on the docs now 👍

@bpleslie bpleslie force-pushed the issue-1673-password-mix-case branch 4 times, most recently from ff690bb to 2a7af9f Compare August 20, 2019 19:55
vbrazo
vbrazo reviewed Aug 20, 2019
View reviewed changes
lib/faker/default/alphanumeric.rb Outdated
@@ -4,7 +4,8 @@ module Faker
class Alphanumeric < Base
class << self
ALPHABET = ('a'..'z').to_a
ALPHANUMS = ALPHABET + (0..9).to_a
NUMBERS = (0..9).to_a
ALPHANUMS = ALPHABET + NUMBERS
Copy link
Member

@vbrazo vbrazo Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We might be redefining/duplicating this constant since Faker::Base already uses something similar. Check this out.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vbrazo good call. I updated to use the constants from the base class.

vbrazo
vbrazo reviewed Aug 20, 2019
View reviewed changes
doc/default/lorem.md
Faker::Lorem.characters #=> "uw1ep04lhs0c4d931n1jmrspprf5wrj85fefue0y7y6m56b6omquh7br7dhqijwlawejpl765nb1716idmp3xnfo85v349pzy2o9rir23y2qhflwr71c1585fnynguiphkjm8p0vktwitcsm16lny7jzp9t4drwav3qmhz4yjq4k04x14gl6p148hulyqioo72tf8nwrxxcclfypz2lc58lsibgfe5w5p0xv95peafjjmm2frkhdc6duoky0aha"
Faker::Lorem.characters(number: 10) #=> "ang9cbhoa8"
Faker::Lorem.characters(number: 10, min_alpha: 4) #=> "ang9cbhoa8"
Faker::Lorem.characters(number: 10, min_alpha: 4, min_numeric: 1) #=> "ang9cbhoa8"
Copy link
Member

Choose a reason for hiding this comment

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

Thanks 👍

Zeragamba
Zeragamba suggested changes Aug 20, 2019
View reviewed changes
lib/faker/default/alphanumeric.rb Outdated Show resolved Hide resolved
@bpleslie bpleslie force-pushed the issue-1673-password-mix-case branch 2 times, most recently from 97b9cee to 9e8feb4 Compare August 23, 2019 21:46
@bpleslie
Copy link
Member Author

@Zeragamba @vbrazo I believe I have addressed all of your feedback. Let me know if you see anything else 👍

@bpleslie bpleslie force-pushed the issue-1673-password-mix-case branch from 9e8feb4 to 76566d9 Compare August 23, 2019 22:16
@vbrazo vbrazo added this to the 2.2.0 milestone Aug 25, 2019
vbrazo added a commit that referenced this pull request Aug 25, 2019
@vbrazo
Add #1694
2524525
@vbrazo
Copy link
Member

vbrazo commented Aug 26, 2019

@bpleslie could you rebase and fix the conflicts?

@bpleslie bpleslie force-pushed the issue-1673-password-mix-case branch from 76566d9 to 0e8439f Compare August 26, 2019 16:31
@bpleslie bpleslie force-pushed the issue-1673-password-mix-case branch from 0e8439f to 942091a Compare August 26, 2019 16:54
@bpleslie
Copy link
Member Author

@vbrazo we should be all set here

@vbrazo vbrazo merged commit 80e4584 into faker-ruby:master Aug 27, 2019
vbrazo added a commit that referenced this pull request Aug 28, 2019
@vbrazo
Bump version to 2.2.0 (#1704)
dc9e62f 
* Bump to version 2.2.0

* Add #1694
@sanikkenway sanikkenway mentioned this pull request Aug 30, 2019
Merged
@bpleslie bpleslie deleted the issue-1673-password-mix-case branch September 10, 2019 00:00
michebble pushed a commit to michebble/faker that referenced this pull request Feb 16, 2020
@bpleslie @michebble
[1673] Ensure mix_case returns at least one lower and one upper case …
3ac84b2 
…letter (faker-ruby#1694)
michebble pushed a commit to michebble/faker that referenced this pull request Feb 16, 2020
@vbrazo @michebble
Bump version to 2.2.0 (faker-ruby#1704)
b904b5a 
* Bump to version 2.2.0

* Add faker-ruby#1694
@Zeragamba Zeragamba mentioned this pull request May 26, 2020
Closed
davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
@bpleslie @vbrazo
[1673] Ensure mix_case returns at least one lower and one upper case …
54bb9fe 
…letter (faker-ruby#1694)
davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
@vbrazo
Bump version to 2.2.0 (faker-ruby#1704)
6ef71bb 
* Bump to version 2.2.0

* Add faker-ruby#1694
@thdaraujo thdaraujo mentioned this pull request Jul 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vbrazo vbrazo vbrazo left review comments

@Zeragamba Zeragamba Zeragamba requested changes

Assignees

@bpleslie bpleslie

Labels
🐞 Issue: Bug
Projects
None yet
Milestone
2.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@bpleslie @Zeragamba @vbrazo