Skip to content

Refactoring: Menu Repository #111

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 3 commits into from
Aug 25, 2020
Merged

Refactoring: Menu Repository #111

merged 3 commits into from
Aug 25, 2020

Conversation

lbajsarowicz
Copy link
Contributor

  1. $model->save() and $model->delete() breaks SOLID principle of single responsibility. Although it's not introduced to Coding Standard yet, the Resource Model should be used for operating with the database. Do not use getResource()->save() in Model magento/magento-coding-standard#187
  2. Add Exceptions to the interface, as these are part of interface API.
  3. Improve verbosity and readability of the Repository (Object - > Menu etc.)

@Igloczek
Copy link
Contributor

Internal ticket DEV-69361

@TM18
Copy link
Contributor

TM18 commented Aug 25, 2020

@lbajsarowicz was this ticket closed on purpose?

@lbajsarowicz
Copy link
Contributor Author

A month passed by without any feedback.
I'm going to fork and develop the MegaMenu on my own.

@mcjwsk
Copy link
Contributor

mcjwsk commented Aug 25, 2020

Łukasz, please don't be offended! We are glad to see your pull requests. We promise to improve. Let's develop the module together.

@lbajsarowicz
Copy link
Contributor Author

No emotions involved. It's just maintainability as I regularly review all my PRs to check the progress.
If you like - just do the Cherry Pick and proceed, I need to have an easy way to extend your module, but to be able to do so - I need to clean up the rubbish that is already there. When reviewing cleanup takes > 30 days, it's going to take months to introduce serious changes to the code.

@mcjwsk
Copy link
Contributor

mcjwsk commented Aug 25, 2020

Okay, seems reasonable, but give us a second chance. I appreciate your involvement and I want to have you as a contributor here :).

@lbajsarowicz
Copy link
Contributor Author

giphy

Don't make it hang too long.

@TM18 TM18 merged commit 849a9e8 into SnowdogApps:develop Aug 25, 2020
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.

4 participants