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

Feat(LuaEngine): Add FormatQuery to all method sending message #247

Closed
wants to merge 2 commits into from

Conversation

iThorgrim
Copy link

@iThorgrim iThorgrim commented Jan 28, 2025

📝 Description

This PR introduces Eluna::FormatText, a new utility function that allows dynamic text formatting for various message-related methods like SendSysMessage, SendNotification, and SendWorldMessage. This function replaces {} placeholders with provided arguments, improving flexibility and readability.

Changes

  • Added Eluna::FormatText: Handles string interpolation dynamically.
  • Updated message functions (SendSysMessage, SendNotification, SendWorldMessage, etc.) to support formatted arguments.
  • Ensures backward compatibility with existing scripts while enhancing usability.

🆕 Modified Methods Overview

1. SendNotification()

Send Notification to player

Example

player:SendNotification("Hello, {}!", player:GetName()) -- Outputs: "Hello, PlayerName!"

2. SendWorldMessage()

Send Mesage to All Player

Example

SendWorldMessage("Server restart in {} minutes.", 5) -- Outputs: "Server restart in 5 minutes."

@Kaev
Copy link
Member

Kaev commented Feb 7, 2025

I don't really see any benefit over using string.format in these cases

55Honey
55Honey previously approved these changes Feb 9, 2025
@55Honey
Copy link

55Honey commented Feb 9, 2025

I don't really see any benefit over using string.format in these cases

@Kaev can you think of an example for this with the existing implementation?

@55Honey 55Honey dismissed their stale review February 9, 2025 18:00

Second thoughts

@Kaev
Copy link
Member

Kaev commented Feb 9, 2025

I don't really see any benefit over using string.format in these cases

@Kaev can you think of an example for this with the existing implementation?

player:SendNotification(string.format("Hello, %s!", player:GetName())) -- Outputs: "Hello, PlayerName!"

SendWorldMessage(string.format("Server restart in %d minutes.", 5)) -- Outputs: "Server restart in 5 minutes."

This should work for basically every case (and even more when it comes to formatting decimal and floating point numbers), not just in a handful of functions where it might be implemented or not like the suggested solution.

Official Lua docs: https://www.lua.org/pil/20.html
Tutorial for string.format: https://www.tutorialspoint.com/string-format-function-in-lua-programming

@iThorgrim
Copy link
Author

I don't really see any benefit over using string.format in these cases

@Kaev can you think of an example for this with the existing implementation?

player:SendNotification(string.format("Hello, %s!", player:GetName())) -- Outputs: "Hello, PlayerName!"

SendWorldMessage(string.format("Server restart in %d minutes.", 5)) -- Outputs: "Server restart in 5 minutes."

This should work for basically every case (and even more when it comes to formatting decimal and floating point numbers), not just in a handful of functions where it might be implemented or not like the suggested solution.

Official Lua docs: https://www.lua.org/pil/20.html Tutorial for string.format: https://www.tutorialspoint.com/string-format-function-in-lua-programming

I know it exists, but it's for the same purpose as WorldDBQuery / WorldDBExecute, to add a certain quality of life :)

@Kaev
Copy link
Member

Kaev commented Feb 10, 2025

I don't really see any benefit over using string.format in these cases

@Kaev can you think of an example for this with the existing implementation?

player:SendNotification(string.format("Hello, %s!", player:GetName())) -- Outputs: "Hello, PlayerName!"
SendWorldMessage(string.format("Server restart in %d minutes.", 5)) -- Outputs: "Server restart in 5 minutes."
This should work for basically every case (and even more when it comes to formatting decimal and floating point numbers), not just in a handful of functions where it might be implemented or not like the suggested solution.
Official Lua docs: https://www.lua.org/pil/20.html Tutorial for string.format: https://www.tutorialspoint.com/string-format-function-in-lua-programming

I know it exists, but it's for the same purpose as WorldDBQuery / WorldDBExecute, to add a certain quality of life :)

I don't know your query PR (and i also just stumbled upon this PR by accident) but the main reason for parameterized queries is usually security (prevent SQL injections) and performance (query caching). If that is not provided, it should be reverted and reworked IMO.

I totally understand where you're coming from with this PR but just for the sake of maintainability, the project shouldn't bloat up with unneccessary stuff like this, especially if that feature already exists in a similar way. That's the main reason why it's harder and just way more work to integrate AC and/or Eluna for AC in other tooling (like the modding framework TSWoW to name one big example).

In the end it's just my personal opinion and the project owners decision. Do with it what you want but don't forget that every addition also needs to get maintained sooner or later.

@iThorgrim
Copy link
Author

I don't know your query PR (and i also just stumbled upon this PR by accident) but the main reason for parameterized queries is usually security (prevent SQL injections) and performance (query caching). If that is not provided, it should be reverted and reworked IMO.

But I understand the arguments, there's no point in constantly reinventing the wheel, I see it as quality of life for developers.

That's the main reason why it's harder and just way more work to integrate AC and/or Eluna for AC in other tooling (like the modding framework TSWoW to name one big example).

That's why I'm working on a version of TSWow for AzerothCore :)

@iThorgrim
Copy link
Author

So what's the point? Do I delete this PR or not? What exactly do you want to do ? @Kaev

@Kaev
Copy link
Member

Kaev commented Feb 16, 2025

So what's the point? Do I delete this PR or not? What exactly do you want to do ? @Kaev

I'm not affiliated to the project, don't ask me. I just shared my opinion why i wouldn't bloat the project up with this.

@55Honey
Copy link

55Honey commented Feb 16, 2025

Thanks for the input. With the information currently available, I don't see the advantage over the existing possibilities.

@iThorgrim iThorgrim closed this Feb 17, 2025
@iThorgrim iThorgrim deleted the MessageFormat branch February 17, 2025 12:05
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