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

ID should only exist in factories #191

Merged
merged 6 commits into from
Sep 11, 2015
Merged

ID should only exist in factories #191

merged 6 commits into from
Sep 11, 2015

Conversation

calclavia
Copy link
Member

It doesn't make sense to have getID() in block, because you can have different instance of block, and have them considered as different types.

It makes more sense to have IDs in factories, because factories are what determines the type of block/entity/item, not the instance itself.

E.g: It doesn't make sense as we're registering instances of TE essentially as a specific type.

Before

blockManager.register(constructor);

ID used to be specified in block instances, which does not make sense.

After

New registration pattern for blocks, items and entities:

blockManager.register(id, constructor);

Example:

blockManager.register("stone", BlockStone::new);

TODO: #196

@calclavia calclavia added enhancement Nice to have features breaking change This is going to break stuff labels Sep 10, 2015
@calclavia calclavia added this to the v0.1.0 milestone Sep 10, 2015
@calclavia calclavia self-assigned this Sep 10, 2015
@calclavia calclavia added the in progress Pull requests that are not yet ready to be merged and issues that are being worked on. label Sep 11, 2015
@calclavia calclavia force-pushed the enhancement/identifier branch from 828cbc9 to e85c659 Compare September 11, 2015 19:16
@codecov-io
Copy link

Current coverage is 12.81%

Merging #191 into master will increase coverage by +0.40% as of fcb82ed

@@            master    #191   diff @@
======================================
  Files          389     391     +2
  Stmts        10383   10411    +28
  Branches      1486    1488     +2
  Methods          0       0       
======================================
+ Hit           1289    1334    +45
  Partial         76      76       
+ Missed        9018    9001    -17

Review entire Coverage Diff as of fcb82ed

Powered by Codecov. Updated on successful CI builds.

@calclavia
Copy link
Member Author

PR Complete. Tested and working.

@Kubuxu, @RX14, @Smesper @balthatrix

calclavia added a commit that referenced this pull request Sep 11, 2015
@calclavia calclavia merged commit 5e0a9a8 into master Sep 11, 2015
@calclavia calclavia removed the in progress Pull requests that are not yet ready to be merged and issues that are being worked on. label Sep 11, 2015
@calclavia calclavia deleted the enhancement/identifier branch September 11, 2015 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This is going to break stuff enhancement Nice to have features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants