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: Avoid globals starting with 'i/' #556

Merged
merged 3 commits into from
Apr 8, 2022
Merged

Fix: Avoid globals starting with 'i/' #556

merged 3 commits into from
Apr 8, 2022

Conversation

StraToN
Copy link
Collaborator

@StraToN StraToN commented Mar 26, 2022

Copy link
Collaborator

@BHSDuncan BHSDuncan left a comment

Choose a reason for hiding this comment

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

More of me being pedantic. :)

@dploeger
Copy link
Collaborator

@StraToN I'm confused. Wouldn't it make more sense to check in inventory_add whether the given global_Id doesn't have a "/" in its name?

@StraToN
Copy link
Collaborator Author

StraToN commented Mar 28, 2022

@dploeger That would also make sense but in this PR I wanted above all to forbid a global having 2 / characters in its name. Since inventory_add calls set_global, I intended to catch all cases there.

@dploeger
Copy link
Collaborator

Shouldn't have a global no / in the name at all? Even i/ is only a hack to work with inventory items. 🤔

@StraToN
Copy link
Collaborator Author

StraToN commented Mar 28, 2022

We could indeed modify the global manager to separate globals from inventory globals instead. Technically, nothing blocks the use of / characters in globals. And I'm just thinking that could actually be useful to keep, so that gamedevs can kind of "classify" their globals.

Which would advocate for a change in inventory_add instead as you mentioned.

I'll change that tomorrow morning.

@StraToN StraToN requested a review from BHSDuncan March 29, 2022 12:33
@balloonpopper balloonpopper self-requested a review April 8, 2022 07:10
@StraToN StraToN changed the title Fix: Avoid globals with more than 1 occurrence of '/' Fix: Avoid globals starting with 'i/' Apr 8, 2022
@StraToN StraToN merged commit 05f139e into develop Apr 8, 2022
@StraToN StraToN deleted the issue-153 branch April 8, 2022 19:20
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.

Multiple errors with inventory items look and use
4 participants