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

refactor: Use variable name on toString in component_test.dart #2377

Merged
merged 4 commits into from
Mar 1, 2023

Conversation

luanpotter
Copy link
Member

Description

This is a cleanup identified on this issue: #2308
Using an amazing unused-code tooling
Now, since Flame is a public API, unused code might not be trivial - it might just mean untested code.

In this case, the unused code is in the tests, which likely means it is indeed necessary.
I tried to infer the intent of the author here, and even considered adding the names of the components to the events, but the events are already always per component, so that was meaningless. I also thought it might be a missing toString, that they would wish to show the component name for debugging purposes, but I didn't see any asserts/logs on the component objects themselves (just the events).
So I concluded there is no reason to keep this field, but lmk if anyone disagrees and I can bring it back and add some function to it.

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

@luanpotter luanpotter marked this pull request as ready for review March 1, 2023 02:43
@spydon spydon requested a review from st-pasha March 1, 2023 09:30
@st-pasha
Copy link
Contributor

st-pasha commented Mar 1, 2023

The name field was for debugging purposes: if some test breaks, you can set up a breakpoint and see clearly which component is which. Perhaps a toString() method would be helpful here?

@luanpotter luanpotter changed the title refactor: Remove unused variable "name" from component_test.dart refactor: Use variable name on toString in component_test.dart Mar 1, 2023
@luanpotter
Copy link
Member Author

Updated!

@spydon spydon merged commit f5c0e5e into main Mar 1, 2023
@spydon spydon deleted the luan.unused-1 branch March 1, 2023 17:03
st-pasha pushed a commit that referenced this pull request Mar 2, 2023
This is a cleanup identified on this issue: #2308
Using an amazing unused-code tooling
Now, since Flame is a public API, unused code might not be trivial - it might just mean untested code.

In this case, the unused code is in the tests, which likely means it is indeed necessary.
I tried to infer the intent of the author here, and even considered adding the names of the components to the events, but the events are already always per component, so that was meaningless. I also thought it might be a missing toString, that they would wish to show the component name for debugging purposes, but I didn't see any asserts/logs on the component objects themselves (just the events).
So I concluded there is no reason to keep this field, but lmk if anyone disagrees and I can bring it back and add some function to it.
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