-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Add Faker::Alphanumeric #1302
Add Faker::Alphanumeric #1302
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. I just left a few comments.
Did you know that we have a Faker::String object? What do you think about adding these methods to the Faker::String?
doc/alphanum.md
Outdated
@@ -0,0 +1,9 @@ | |||
# Faker::Alphanum | |||
|
|||
Available since version 0.3.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be It might be available in the next version.
Don't we already have this in Faker::Lorem.characters? |
@stympy hmm you're right. Thanks! |
Hmmm, alright if it's already in Something like [...]
- [Faker::LordOfTheRings](doc/lord_of_the_rings.md)
- [Faker::Lorem](doc/lorem.md) (Lorem ipsum strings, words, and random alphanumeric strings)
- [Faker::LoremFlickr](doc/lorem_flickr.md)
[...] |
Since the functionality is the same in both cases, I think a possible solution would be creating an |
Right, and lorem lacks the "letters only" generator. I'll do it for next weekend, i'm taking some days off. |
I updated |
Is this PR ok for you now ? |
what other methods do you believe we could implement in the |
@vbrazo I don't really know, what kind of string can we put here that are simple enough to fit here, but not in another class ? It's really generic, but maybe we can include an "uppercase" option, and maybe a mix of lower/uppercase option too, in order to have more choice... |
ok cool. We've added several modules recently based on contributors' suggestions. Just asking you because I'm afraid of adding another module that could eventually have only two methods. We'll come with namespaces soon and then this concern will disappear. I agree and believe that |
lib/faker/alphanum.rb
Outdated
when Range then rand value | ||
else value | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to remove resolve
in Faker::Lorem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hum.. you're using resolve in other internal methods there. Nevermind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually you could call it Faker::Lorem.resolve
instead of duplicating the lines or do the inverse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing for ALPHANUMS
Would you consider using It seems that (most?) other Fakers are not abbreviated. |
I updated the class name to |
@vbrazo thanks for the changes ! |
* Add Faker::Alphanum * Update and rename alphanum.md to alphanumeric.md * Update README.md * Remove duplications * Minor change
Simple alphanumeric generator for random alphanumeric strings. I think it's useful to have the ability to create simple (unique) strings.
From the documentation: