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

Global variable and default value for resource names #3363

Merged
merged 21 commits into from
Jun 30, 2024

Conversation

Nico8340
Copy link
Contributor

Description

This pull request creates a global variable named resourceName, which always contains the name of the current resource, and sets the current resource as the default value to the function getResourceName, so if it is not defined as an argument, it is based on that, like in case of other similar functions. Closes #2927.

Example

addEventHandler("onResourceStart", resourceRoot,
    function()
        local myResourceName = resourceName or getResourceName()
        local myResourceMessage = string.format("Resource successfully started: %s", myResourceName)

        outputChatBox(myResourceMessage)
    end
)

A global variable named resourceName that always contains the name of the given resource.
Adds a default value to the getResourceName function, if no resource is defined, it handles the current resource, like other similar functions.
@Nico8340
Copy link
Contributor Author

@TracerDS
Thanks for the suggestions.
Although I didn't write this piece of code, I updated it according to modern standards.

@Nico8340 Nico8340 requested a review from TracerDS April 14, 2024 16:28
@TracerDS
Copy link
Contributor

@TracerDS Thanks for the suggestions. Although I didn't write this piece of code, I updated it according to modern standards.

You could also format it to use ArgumentParser, but I think it would be best for a separate PR somewhere in the future

Copy link
Contributor

@TracerDS TracerDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@Fernando-A-Rocha Fernando-A-Rocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job

Copy link
Contributor

@TracerDS TracerDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you still want to merge these changes, please use ArgumentParser.
A small refactor wouldnt be so bad here

Client/mods/deathmatch/logic/luadefs/CLuaResourceDefs.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/luadefs/CLuaResourceDefs.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@Fernando-A-Rocha Fernando-A-Rocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Copy link
Contributor

@TracerDS TracerDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👌

@Fernando-A-Rocha
Copy link
Contributor

Bump 😈

@tederis tederis added the enhancement New feature or request label Jun 18, 2024
This reverts commit cfe8b1e.
Copy link
Contributor

@Fernando-A-Rocha Fernando-A-Rocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK LGTM

@Dutchman101
Copy link
Member

Enough code reviews and approvals have been submitted, thanks!

@Dutchman101 Dutchman101 merged commit cd88950 into multitheftauto:master Jun 30, 2024
6 checks passed
botder added a commit that referenced this pull request Jun 30, 2024
This reverts commit cd88950.
The changes in the commit primarily broke map editor and other resources.
@TracerDS TracerDS mentioned this pull request Jun 30, 2024
1 task
@Nico8340 Nico8340 deleted the resname branch June 30, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make getResourceName take current resource as a default argument
5 participants