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

Divide et impera #190

Merged
merged 3 commits into from
Sep 26, 2024
Merged

Divide et impera #190

merged 3 commits into from
Sep 26, 2024

Conversation

kewinrausch
Copy link
Contributor

Changes Proposed:

  • A single file implementation split into multiple files. Each class has its own file, which allows to split complexity of the module into multiple black boxes that can be better understood and extended in the future.

Issues Addressed:

  • No issues addressed

SOURCE:

Tests Performed:

  • Compiles with no error and no deprecation warning
  • Tested into private server into a dungeon instances; seems to work as old version since no changes has been performed on the logic of the module and balancing.

How to Test the Changes:

  1. Pull the changes
  2. Compile the server
  3. Enter and complete and instance; i did not touch internal module mechanism except for splitting the classes into multiple files and some decorations.

Take you time to confirm that the module work as intended.
I'm personally performing additional tests to be sure the balancing is not affected. I expect no problem since i just moved code around files and did not touch the code inside the classes and procedures.

…o changes on the module internal mechanism, only code movement from a file to another
@Helias
Copy link
Member

Helias commented Sep 21, 2024

it's a big PR, not sure if I have the time to review it soon :(

@kewinrausch
Copy link
Contributor Author

Do not worry, i have no rush or bad feelings about it. Take all the time you need.
I will keep playing using this and test out if something is broken on my side.

@Yehonal
Copy link
Member

Yehonal commented Sep 26, 2024

I think we should merge this asap, maybe we can "versioning" this module and add to the readme the commit/link to the version before this as a stable version, and this will stay in "beta" until we ensured that everything is working

unless there's someone that wants to test it extensively right now, but I'm afraid that the more time we wait the worse it will be to resolve the conflicts with other incoming PRs

@Helias
Copy link
Member

Helias commented Sep 26, 2024

@kewinrausch you could add a note in the README.md about this PR, saying that from this PR the module could be "unstable" and we'll merge the PR ;-)

I appreciate the big effort here, this note it's only to keep track of this

@kewinrausch
Copy link
Contributor Author

I did as you requested.

Wouldn't it be better to create also a tag or a branch pointing to the stable version right now? Once enough time has passed and enough test have been performed the branch can be removed to maintain only the master one.

README.md Outdated Show resolved Hide resolved
@Helias
Copy link
Member

Helias commented Sep 26, 2024

done ✅

@Helias Helias merged commit 4619c9e into azerothcore:master Sep 26, 2024
1 check passed
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