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: Remove dead code #2570

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

aragon999
Copy link
Contributor

1. Why is this change necessary?

Cleanup dead code.

The code which is removed, is not needed, since on the one hand loadModule is a private method, which is only called if this->modules_container[$name] is null. On the other hand, $name will never be a valid path, since the getModule method always appends an s in front of the $name.

2. Checklist

  • I have written tests and verified that they fail without my change
  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfil them.

@aragon999 aragon999 force-pushed the refactor/remove-modules-dead-code branch from dc81db2 to 5082ea5 Compare August 7, 2023 00:03
@mitelg
Copy link
Contributor

mitelg commented Aug 24, 2023

hey @aragon999

thanks for your PR, it looks good to me 👍

please rebase your branch

@aragon999 aragon999 force-pushed the refactor/remove-modules-dead-code branch from 5082ea5 to b2e3394 Compare August 24, 2023 13:25
@github-actions
Copy link

Warnings
⚠️ The Pull Request doesn't contain any changes to the Upgrade file

@mitelg mitelg removed the Incomplete label Aug 24, 2023
@rathmerdominik rathmerdominik self-requested a review August 24, 2023 14:06
@mitelg mitelg merged commit d95bba2 into shopware5:5.7 Aug 24, 2023
15 checks passed
@mitelg
Copy link
Contributor

mitelg commented Aug 24, 2023

thanks for your contribution @aragon999 👍🎉💙

@aragon999 aragon999 deleted the refactor/remove-modules-dead-code branch August 24, 2023 14:14
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