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

Adds FFaker::Number:leading_zero_number #538

Closed
wants to merge 1 commit into from

Conversation

professor
Copy link
Contributor

I'm on a project that has used both Faker and FFaker for a long time. We're now standardizing on FFaker. For much of the code, I'm able to use FFaker API calls. Occasionally, I come across a concept that I'd like to see in FFaker, hence this PR.

This adds a FFaker::Number.leading_zero_number.

Note that this would return a string, not an integer. I'm not sure if that breaks any design constraints with FFaker (as compared to Faker). We only use it once, so I'm happy to inline the code in our app. I'm providing it here for anyone else transitioning from Faker to FFaker.

lib/ffaker/number.rb Outdated Show resolved Hide resolved
test/test_number.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@AlexWayfer AlexWayfer left a comment

Choose a reason for hiding this comment

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

I don't understand the meaning of this method.

And I don't understand Faker motivation for:

Where are discussions, reasons?

I see this algorithm very strange and rare-case for me. What do you need it for? Why not a regular number? Why 10 digits is default? Why only one guaranteed zero?

Copy link
Member

@marocchino marocchino left a comment

Choose a reason for hiding this comment

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

I realized you don't add :leading_zero_number to assert_methods_are_deterministic.
please add it?

@professor
Copy link
Contributor Author

I realized you don't add :leading_zero_number to assert_methods_are_deterministic.
please add it?

Fixed

@professor
Copy link
Contributor Author

I don't understand the meaning of this method.

In many systems, there are number (invoice numbers, account numbers) where there are leading 0's before the random numbers.

My code base currently only uses this once and instead of typing

FFaker::Number.leading_zero_number(digits: 9)

I could replace it with

"0{FFaker::Number.number(digits: 8)}"

@AlexWayfer
Copy link
Contributor

In many systems, there are number (invoice numbers, account numbers) where there are leading 0's before the random numbers.

Yeah, I don't understand why it starts with 0. Why not with 1, 9, 111? It seems very specific for me, I've never seen or done such thing. I could only saw something like a company has ranges, and invoices are incrementing, and it looks like 10004567, and can be 10876543 later.

So… if there any reason for single leading zero, standard — I'd like to know. Public reference. If no, if it's something specific and unexplained — I'm against a dedicate such method in library and for in-place simple implementations (just add 1 character).

For me it seems like "let's add a method, which returns Lorem Ipsum, but in downcase, instead of external downcase method call".

I hope you understand.

@professor
Copy link
Contributor Author

I'm fine with the pushback on this PR and will close it. Leading zeros can be seen in some of the older systems from the 80's and 90's where systems were memory and disk space were at a premium and the programmers wanted to prepare for growth. It's what I grew up with in computing, and I'd hope a modern system wouldn't implement it.

@professor professor closed this Sep 7, 2023
@AlexWayfer
Copy link
Contributor

Oh… interesting. Yeah, I'm not familiar with this, being a younger man than these systems. If it's only the reason, I mean type of such data — then yes, I'm for not adding this as kind of outdated. It'd be strange to drop 4-years-old Ruby versions support, but support 30-years old data format, which can be implemented very simply by those who needed.

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.

None yet

3 participants