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

Mixin package refactor #247

Closed
wants to merge 2 commits into from
Closed

Mixin package refactor #247

wants to merge 2 commits into from

Conversation

Alexdoru
Copy link
Member

Moved all mixins from mixins package to mixin/mixins package so we can put the mixins related hooks and other classes in the mixin package and not have them sit in a common package

@github-actions
Copy link
Contributor

Warning: 2 uncommitted changes
#248

This was referenced Sep 19, 2023
@Dream-Master Dream-Master requested review from a team September 21, 2023 14:42
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.

Tbh I don't see the point. Just changing the common package to a different name should suffice, if at all. Thoughts, @mitchej123?

@Alexdoru
Copy link
Member Author

when you make mixins you sometime needs external interfaces to call methods or you create class hooks and it makes sense to put them under a mixin package that's all

@glowredman
Copy link
Member

I completely understand what you mean, I just don't agree. When I see a package called "mixin(s)" I expect everything inside to be directly responsible for bytecode manipulation via the Mixins framework and nothing else.

@Alexdoru
Copy link
Member Author

So are you saying that when you see such a refactored package you don't understand where the mixins are ?
image

And you would rather have mixin stuff spread accross two package with a second package that isn't even named mixin ?

image

@glowredman
Copy link
Member

So are you saying that when you see such a refactored package you don't understand where the mixins are ?

I am not a machine, I can work things out when they don't meet my expectations.

And you would rather have mixin stuff spread accross two package with a second package that isn't even named mixin ?

Yes because one of these doesn't actually contain mixins, the classes are just somewhat related. ItemBlocks don't belong in an item(s) package either. They are related to blocks but aren't actually blocks.

@Alexdoru
Copy link
Member Author

I guess we disagreee, @mitchej123 what do you think ?

@mitchej123
Copy link
Contributor

I'm not a fan the mixins being nested, so I'm with glowredman on this one

@Alexdoru Alexdoru closed this Sep 24, 2023
@Dream-Master Dream-Master deleted the refector-mixin-package branch September 24, 2023 18:10
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