Skip to content

[9.x] Default address methods #35534

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

Merged
merged 2 commits into from
Dec 8, 2020

Conversation

jasonmccreary
Copy link
Contributor

@jasonmccreary jasonmccreary commented Dec 8, 2020

Upon using these two methods for the first time recently, my expectation was they would automatically set the column name for me given their specific method names.

I addressed this tiny paper-cut by defaulting the column name inline with Laravel's naming convention (snake case) for both the ipAddress and macAddress methods.

Before

$table->ipAddress('ip_address');
$table->macAddress('mac_address');

After equivalent

$table->ipAddress();
$table->macAddress();

Note: I considered this a breaking change since it updates the method signatures. However, this could be defaulted in another way to merge into Laravel 8.x if desired.

@jasonmccreary
Copy link
Contributor Author

The failure seems unrelated. However, let me know if this somehow affected those tests.

@GrahamCampbell GrahamCampbell changed the title Default address methods [9.x] Default address methods Dec 8, 2020
@GrahamCampbell
Copy link
Member

Yep, we're aware master is failing, due to pending upstream (testbench) changes. Should be sort out soon. :)

@GrahamCampbell
Copy link
Member

Hmmm, I don't think I like these defaults.

@jasonmccreary
Copy link
Contributor Author

@GrahamCampbell I felt there were sensible/predictable given Laravel's naming convention and the current method names. I'm open to whatever. These were really just for convenience. One can always pass the argument.

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.

3 participants