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

Refactor references NS to use consistent and explicit variable and method names #675

Merged
merged 8 commits into from
Jul 27, 2020

Conversation

georgehristov
Copy link
Collaborator

Related to solving #662

@georgehristov georgehristov force-pushed the refactor/references-var-names branch from 0faf920 to 47c3d37 Compare July 25, 2020 15:25
@georgehristov georgehristov force-pushed the refactor/references-var-names branch from 47c3d37 to b985e52 Compare July 25, 2020 15:42
src/Reference.php Outdated Show resolved Hide resolved
src/Reference.php Outdated Show resolved Hide resolved
src/Reference.php Outdated Show resolved Hide resolved
src/Reference.php Outdated Show resolved Hide resolved

return $p;
return new Persistence\ArrayOfStrings([$this->getTableAlias() => $row ? [1 => $row] : []]);
Copy link
Member

Choose a reason for hiding this comment

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

why not 20? Isn't this ID?

$ourModel = $this->getOurModel();

return $this->getTheirModel($defaults)->addCondition(
$this->their_field ?: ($ourModel->table . '_' . ($ourModel->id_field ?: 'id')),
Copy link
Member

Choose a reason for hiding this comment

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

we should have more like two protected methods for build and parse (I think related with table alias regexes above)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also here - feel free to implement your ideas in this PR

Copy link
Member

Choose a reason for hiding this comment

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

related with #675 (comment) , this should be probably solved in this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR is about pure renaming. Let's merge this PR and you look into it when you have time and willingness for it.

@georgehristov georgehristov force-pushed the refactor/references-var-names branch from b2c69db to 5196ace Compare July 26, 2020 09:22
@georgehristov georgehristov force-pushed the refactor/references-var-names branch from 8ceefb5 to e2121cb Compare July 26, 2020 09:31
@georgehristov
Copy link
Collaborator Author

@mvorisek let's merge this now as pure var name refactoring.
We also still need to place this in the "correct" NS under Model\Reference but it is a matter of further PRs

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.

Very good work on making code more readable. Especially their/our model. That was always a pain to not mix these two.

@georgehristov georgehristov merged commit bc82230 into develop Jul 27, 2020
@georgehristov georgehristov deleted the refactor/references-var-names branch July 27, 2020 17:13
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