-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Mexico #979
base: master
Are you sure you want to change the base?
Mexico #979
Conversation
src/Faker/Provider/es_MX/Person.php
Outdated
@@ -95,21 +95,28 @@ public static function curp($firstName = null, $lastNameFather = null, $lastName | |||
|
|||
public static function rfc($firstName = null, $lastNameFather = null, $lastNameMother = null, $birthDate = null, $gender = null) |
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.
This really shouldn't be there, but since there are many helpers in Person class that needs to be in Payment class also, I couldn't figure out how to do it better, Maybe Trait, but not all PHP versions support Traits
what is needed for this to be merged? |
src/Faker/Provider/es_MX/Payment.php
Outdated
public static function clabe() | ||
{ | ||
$bank = static::randomElement(static::$bankCodes); | ||
$accountNumber = $bank.static::numerify('##############'); |
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 enclose dot with two spaces
Needs rebase |
Can you squash your 19 commits into one? |
Yes I could, but you should never rewrite history in git |
@fzaninotto i think its easier for you to squash them from git merge options. do you see an 'squash and merge' option at the bottom of this pr? |
Let me put this differently: I don't want to see a merge commit in your PR, because it brings some changes unrelated to your work in the changeset. So what I really want is for you to rebase on master, not merge master in. |
Because rewriting history is great??? I will do it, just know that it is not best thing to do |
dude, please don't teach me how to do web development |
I'm not telling you how to do web development |
tests fail |
src/Faker/Provider/es_MX/Person.php
Outdated
|
||
$lastNameFather = self::removeAccents(self::removePrefixes(mb_strtoupper($lastNameFather ? $lastNameFather : static::randomElement(static::$lastNames)))); | ||
$lastNameMother = self::removeAccents(self::removePrefixes(mb_strtoupper($lastNameMother ? $lastNameMother : static::randomElement(static::$lastNames)))); | ||
$birthDate = $birthDate ? $birthDate:\Faker\Provider\DateTime::dateTimeBetween(); |
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.
This ternary operator is missing its second part.
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.
no it is not
if $birthDate exists then pass it to $birthDate, otherwise take random DateTime::dateTimeBetween()
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.
OK, then it's the code formatting that is wrong. Please add spaces around the colon.
@localheinz |
Fixed code style issues Changed first names origin to wikipedia
Codecov Report
@@ Coverage Diff @@
## master #979 +/- ##
============================================
+ Coverage 56.31% 56.86% +0.54%
- Complexity 2068 2109 +41
============================================
Files 306 308 +2
Lines 4851 4936 +85
============================================
+ Hits 2732 2807 +75
- Misses 2119 2129 +10
Continue to review full report at Codecov.
|
implemented basic Mexico