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

Add Faker::Alphanumeric #1302

Merged
merged 5 commits into from
Sep 20, 2018
Merged

Add Faker::Alphanumeric #1302

merged 5 commits into from
Sep 20, 2018

Conversation

mtancoigne
Copy link
Contributor

@mtancoigne mtancoigne commented Jul 4, 2018

Simple alphanumeric generator for random alphanumeric strings. I think it's useful to have the ability to create simple (unique) strings.

From the documentation:

Faker::Alphanumeric.alpha 10 #=> "zlvubkrwga"
Faker::Alphanumeric.alphanum 10 #=> "3yfq2phxtb"
  • Class
  • New tests
  • New doc
  • Rubocoped

Copy link
Member

@vbrazo vbrazo left a 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.
Copy link
Member

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.

@stympy
Copy link
Contributor

stympy commented Jul 5, 2018

Don't we already have this in Faker::Lorem.characters?

@vbrazo
Copy link
Member

vbrazo commented Jul 5, 2018

@stympy hmm you're right. Thanks!

@vbrazo vbrazo closed this Jul 5, 2018
@mtancoigne
Copy link
Contributor Author

mtancoigne commented Jul 6, 2018

Hmmm, alright if it's already in Lorem, but how can we make this more obvious ? The generators list is pretty big, and I think that Lorem is not a really good place for random chars. maybe writing side notes near the generator link in the readme ? (i'm sure we can't move the generator from lorem to string for retro-compatibility)

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)
[...]

@vbrazo
Copy link
Member

vbrazo commented Jul 6, 2018

Since the functionality is the same in both cases, I think a possible solution would be creating an alias.

@vbrazo vbrazo reopened this Jul 6, 2018
@mtancoigne
Copy link
Contributor Author

Right, and lorem lacks the "letters only" generator. I'll do it for next weekend, i'm taking some days off.

@mtancoigne mtancoigne changed the title Add Faker::Alphanum WIP: Add Faker::Alphanum Jul 6, 2018
@mtancoigne
Copy link
Contributor Author

I updated Lorem.characters to return Alphanum.alphanum, I think it makes sense to do it this way. Tell me if you prefer the call to be inverted.

@mtancoigne mtancoigne changed the title WIP: Add Faker::Alphanum Add Faker::Alphanum Jul 11, 2018
@mtancoigne
Copy link
Contributor Author

Is this PR ok for you now ?

@vbrazo
Copy link
Member

vbrazo commented Aug 29, 2018

what other methods do you believe we could implement in the Faker::AlphaNum? @mtancoigne it doesn't need to be official and done in the PR. I'm just curious.

@mtancoigne
Copy link
Contributor Author

mtancoigne commented Aug 31, 2018

@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...

@vbrazo
Copy link
Member

vbrazo commented Sep 4, 2018

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 Alphanum could have other methods @mtancoigne

when Range then rand value
else value
end
end
Copy link
Member

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

Copy link
Member

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

Copy link
Member

@vbrazo vbrazo Sep 4, 2018

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

Copy link
Member

Choose a reason for hiding this comment

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

same thing for ALPHANUMS

@josephgrossberg
Copy link

Would you consider using Faker::Alphanumeric instead of Faker::Alphanum?

It seems that (most?) other Fakers are not abbreviated.

@mtancoigne
Copy link
Contributor Author

I updated the class name to Alphanumeric; method alphanum to alphanumeric, but kept alpha method name for letters only. If you have a better name, i'll update this one too.

@vbrazo vbrazo changed the title Add Faker::Alphanum Add Faker::Alphanumeric Sep 17, 2018
@mtancoigne
Copy link
Contributor Author

@vbrazo thanks for the changes !

@vbrazo vbrazo merged commit 1511907 into faker-ruby:master Sep 20, 2018
@mtancoigne mtancoigne deleted the feature/alphanum branch September 26, 2018 10:14
davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
* Add Faker::Alphanum

* Update and rename alphanum.md to alphanumeric.md

* Update README.md

* Remove duplications

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

Successfully merging this pull request may close these issues.

4 participants