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

exchange reflection field access with mixin accessors #21

Merged
merged 3 commits into from
Jan 29, 2023

Conversation

Alexdoru
Copy link
Member

No description provided.

@Dream-Master Dream-Master requested review from a team January 11, 2023 17:03
Copy link
Member

@glowredman glowredman left a comment

Choose a reason for hiding this comment

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

Have you measured any performance improvements? Otherwise I don't really see the point in changing it.

@Alexdoru
Copy link
Member Author

The mod already has mixins targeting the same classes. It's about making the code clean + This way when we look at the class dump, we know which mod is interacting with those fields.

@Alexdoru
Copy link
Member Author

Also it's using reflection to access a field that was created via mixins... that's not how you properly use mixins

Copy link

@mitchej123 mitchej123 left a comment

Choose a reason for hiding this comment

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

I'm a fan of accessors over reflection. Speed or not it generally reads nicer.

@mitchej123 mitchej123 merged commit e2e2050 into master Jan 29, 2023
@mitchej123 mitchej123 deleted the mixins_my_beloved branch January 29, 2023 00:55
@mitchej123 mitchej123 deleted the mixins_my_beloved branch January 29, 2023 00:55
@Alexdoru
Copy link
Member Author

I'm a fan of accessors over reflection. Speed or not it generally reads nicer.

I am a fan too

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.

None yet

3 participants