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

Remove world:target debug error #137

Merged
merged 3 commits into from
Sep 29, 2024
Merged

Remove world:target debug error #137

merged 3 commits into from
Sep 29, 2024

Conversation

1Axen
Copy link
Contributor

@1Axen 1Axen commented Sep 29, 2024

Brief Description

This PR removes the debug error about world:target needing an index parameter.
The parameter was made optional in #131.

Impact

This will fix compatibility with working code in non debug mode.

* The `index` parameter was made optional in #131
@1Axen 1Axen changed the title Removed world:target debug error Remov world:target debug error Sep 29, 2024
@1Axen 1Axen changed the title Remov world:target debug error Remove world:target debug error Sep 29, 2024
Copy link
Collaborator

@EncodedVenom EncodedVenom left a comment

Choose a reason for hiding this comment

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

For consistency with the World.remove definition below, I believe it would be best to change this to the following instead of outright deleting it:

World.target = function(world, entity, relation, index)
    return world_target(world, entity, relation, index)
end

Copy link
Collaborator

@EncodedVenom EncodedVenom left a comment

Choose a reason for hiding this comment

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

Pending review from @Ukendio this LGTM.

@Ukendio
Copy link
Owner

Ukendio commented Sep 29, 2024

World.remove is most definitely a bug or a classic "I am going to work on this later but forgot to stash". I think we should probably remove the wrapper for both World.remove and World.target

@Ukendio Ukendio merged commit a090dd7 into Ukendio:main Sep 29, 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