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

proper use filter in getTags function #14427

Merged
merged 3 commits into from
Oct 18, 2023

Conversation

carolhmj
Copy link
Contributor

Fix #14424

@carolhmj carolhmj assigned carolhmj and unassigned carolhmj Oct 16, 2023
@carolhmj carolhmj added the bug label Oct 16, 2023
@bjsplat
Copy link
Collaborator

bjsplat commented Oct 16, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

LGTM, let see what @deltakosh thinks and I hope it was a bug and not intended :-)

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 16, 2023

@deltakosh
Copy link
Contributor

I did not write that code but I remember it :)
The story was that the forEach was initially meant to run a piece of code on each selected item. The doc was added later on and was misleading. That being said, I prefer this new version but we need to flag it as breaking change (even if it will probably be minimal)

@carolhmj
Copy link
Contributor Author

Since we're adding as a breaking change anyway, I'm renaming forEach to filter to make its function clearer 🙂

@RaananW
Copy link
Member

RaananW commented Oct 18, 2023

Since we're adding as a breaking change anyway, I'm renaming forEach to filter to make its function clearer 🙂

Changing the name of a variable is not a really breaking change. it doesn't change a thing when calling the function.

@RaananW RaananW enabled auto-merge (squash) October 18, 2023 11:49
@RaananW RaananW merged commit 3db59cf into BabylonJS:master Oct 18, 2023
10 checks passed
@AlexanderMelde
Copy link

Do I understand correctly, the migration of the breaking change is simply the following?
scene.getMeshesByTags(tag, fn); -> scene.getMeshesByTags(tag).map(fn);

@sebavan
Copy link
Member

sebavan commented Oct 24, 2023

Sorry for the misunderstanding here.

Yes to have same behavior you could do scene.getMeshesByTags(tag, fn); -> scene.getMeshesByTags(tag).forEach(fn); as fn is now really use as a predicate like it was intended before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scene.getMeshesByTags doesn't use argument predicate as filter
6 participants