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.

Sign up for GitHub

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

Jump to bottom

Require array in reference/join defaults #820

Merged
merged 3 commits into from
Dec 30, 2020
Merged

Require array in reference/join defaults #820

merged 3 commits into from
Dec 30, 2020

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Dec 30, 2020 •
edited
Loading

hasOne/hasMany was nice with string/object, but allowing string violates seed definition

if we however allow a seed for Model directly (without ['model' => ...]) then it can not be distinguished from type only (array vs. array)

thus this impl. is strict, but the only one simple and reliable

@mvorisek mvorisek changed the title Table must be specified with table key in defaults Require array in reference/join defaults Dec 30, 2020
@mvorisek mvorisek force-pushed the no_str_defaults branch 2 times, most recently from 9e122c4 to c328144 Compare December 30, 2020 00:56
@mvorisek mvorisek mentioned this pull request Dec 30, 2020
Merged
DarkSide666
DarkSide666 approved these changes Dec 30, 2020
View reviewed changes
Copy link
Member

@DarkSide666 DarkSide666 left a comment

Choose a reason for hiding this comment

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

This is quite big BC break, but for consistency and good code sake it should be like that.

One thing to also think about is maybe rename join master_field -> our_field and foreign_field -> their_field? Like we have in references? That would be more consistent then too. Of course that would also be BC break, but ...

@georgehristov
Copy link
Collaborator

georgehristov commented Dec 30, 2020 •
edited
Loading

One thing to also think about is maybe rename join master_field -> our_field and foreign_field -> their_field? Like we have in references? That would be more consistent then too. Of course that would also be BC break, but ...

This idea I would support. For consistency we should also consider ourField and theirField names in both references and joins

@mvorisek
Copy link
Member Author

I have no resources to pursue another renames, but I like that idea to.

@mvorisek mvorisek merged commit 48ca2aa into develop Dec 30, 2020
@mvorisek mvorisek deleted the no_str_defaults branch December 30, 2020 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DarkSide666 DarkSide666 DarkSide666 approved these changes

@georgehristov georgehristov Awaiting requested review from georgehristov

Assignees
No one assigned
Labels
BC-break
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mvorisek @georgehristov @DarkSide666