-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Improving pt_BR Providers #545
Improving pt_BR Providers #545
Conversation
Code extensibility was improved by changing private property to protected; Additional methods to generate some Brazilian documents were included
PhoneNumber was rewritten to enable different phone rules in Brazil. Now it generated area codes, landline and cellphone numbers, obeying the rules for the new digit that was added in the last years (and will still be added in the future). The formats are now local-only, like the original PhoneNumber class.
Conflicts: readme.md
- several CS fixes - improved strategy for random selection of phone types - improved default values for formatting - not overwriting phoneNumber() signature anymore; a phoneNumberCleared() was added instead
- added changes on phoneNumber/phoneNumberCleared - fixed typo
c630bef
to
5322f13
Compare
@@ -90,7 +90,7 @@ class Person extends \Faker\Provider\Person | |||
|
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.
The file permissions have changed, please revert to 644.
It seems the Travis build failed because of composer issues, on PHP 7. |
…aker into improved-pt-cherry-picked
* @param bool $formatted If the number should have dots/slashes/dashes or not. | ||
* @return string | ||
*/ | ||
public function cnpj($formatted = true) |
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.
CNPJ is a Company property, not Person.
Can you move it?
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.
I'm not really sure if we need to create a whole new provider just to generate company documents. Even more if you consider that's a juridic person document, same as the CPF for a physical person.
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.
Here are the company provider.
https://github.com/fzaninotto/Faker/blob/master/src/Faker/Provider/pt_BR/Company.php
A juridic person is a company, how said on Wikipedia, CNPJ is an identification number to companies and legal entities. I think is more logical move this property to company provider.
Eg.
I, as a person, have a CPF
I, as a company, have a CNPJ
I don't know if it make sense for you. Please, tell me what you think about it.
- CNPJ is a company document, thus the move - check digit function was properly named, and moved into a separate file, as it's used by both classes. Furthermore, as the check digit algorithm is particular to Brazil (it's a variation of mod11, with different maximum multipliers given the number length) it was not placed in Faker/Calculator.
- Brazilian RG generator was included, as originally request by @hussani and @ianrodrigues at fzaninotto#488 and fzaninotto#551 - Check digit PHPDoc was included, moving in validation links from CPF and CNPJ generators
c77558b
to
5454bee
Compare
I, particularly, disagree with an RG generator. RG doesn't follows any pattern across the states. While RG's in São Paulo matches |
I guess there's no such a thing as an algorithm to avoid real documents Usually RG is accepted as plain text across systems. On top of that, it's If someone needs a specific state's version, one can simply PR an algorithm
|
Any news on this, @fzaninotto? |
public function testRgFormatIsValid() | ||
{ | ||
$rg = $this->faker->rg(false); | ||
echo $rg."\n"; |
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.
please remove
echo $rg."\n"; | ||
$this->assertRegExp('/\d{8}\d/', $rg); | ||
$rg = $this->faker->rg(true); | ||
echo $rg."\n"; |
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.
please remove
Good PR, I'll merge as soon as you fix the two minor debug statements. |
Whoops! Fixed now. |
@fzaninotto, stuff is fixed ;) |
Thanks! |
New version of #335, with CS and other fixes