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

Fix build, crash, etc. #23

Merged
merged 5 commits into from
Mar 10, 2024
Merged

Fix build, crash, etc. #23

merged 5 commits into from
Mar 10, 2024

Conversation

anzz1
Copy link
Contributor

@anzz1 anzz1 commented Jan 10, 2022

  • Replaced deprecated config api to fix build
  • Fix crash when NumPhrases = 0
  • Add option to enable/disable
  • Add visual effect for successful enchantment

anzz1 and others added 4 commits January 10, 2022 14:09
* Replaced deprecated config api to fix build
* Fix crash when NumPhrases = 0
* Add option to enable/disable
* Add visual effect for successful enchantment
@pangolp
Copy link
Contributor

pangolp commented Mar 10, 2024

Great @Helias, you are full with the modules. 😀😀😀

@Helias
Copy link
Member

Helias commented Mar 10, 2024

Great @Helias, you are full with the modules. 😀😀😀

some weekends I check the modules and I update them hehe, I put all the reusable-workflows ci in all modules

@pangolp
Copy link
Contributor

pangolp commented Mar 10, 2024

some weekends I check the modules and I update them hehe, I put all the reusable-workflows ci in all modules

Brilliant. I had updated several of them more than 6 months ago. But perhaps, some of them were left without updating.

@pangolp
Copy link
Contributor

pangolp commented Mar 10, 2024

Regarding the pull request. I would delete the commented line or, failing that, I would create a configuration, for those who want to enable it. I think the second option is better, and let everyone decide if they want to do it or not. Therefore, if you want, once the pull request is approved, I can make the change, or if you want to do so, you can include it right here.

@Helias
Copy link
Member

Helias commented Mar 10, 2024

Regarding the pull request. I would delete the commented line or, failing that, I would create a configuration, for those who want to enable it. I think the second option is better, and let everyone decide if they want to do it or not. Therefore, if you want, once the pull request is approved, I can make the change, or if you want to do so, you can include it right here.

you could do it in a separate PR, in this PR I am trying to finish fixing the build so that at least it compiles and someone can use it in a way

@pangolp
Copy link
Contributor

pangolp commented Mar 10, 2024

you could do it in a separate PR, in this PR I am trying to finish fixing the build so that at least it compiles and someone can use it in a way

Perfect, we'll do it in another pull request then.

@pangolp
Copy link
Contributor

pangolp commented Mar 10, 2024

You will have to correct the SQL path. Afterwards, you have to explain to me how to edit someone else's pull request, because it is not clear to me what steps I have to do. I think I have permissions to do it, but in this case, if I wanted to edit the path, I wouldn't know how to do it. Unless you create another pull request, merge it, and then update the branch, but I figure there must be another way to do it.

@pangolp pangolp requested review from pangolp and removed request for pangolp March 10, 2024 14:04
@pangolp
Copy link
Contributor

pangolp commented Mar 10, 2024

My goodness, it was difficult for me but I think I understood how it is. 😄😄😄

@Helias Helias merged commit 89fc943 into azerothcore:master Mar 10, 2024
1 check passed
@Helias
Copy link
Member

Helias commented Mar 10, 2024

PR merged, thanks you both!

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