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

[RFC] Typed identifiers #227

Open
SoniEx2 opened this issue Oct 24, 2016 · 4 comments
Open

[RFC] Typed identifiers #227

SoniEx2 opened this issue Oct 24, 2016 · 4 comments
Assignees
Labels
breaking change This is going to break stuff
Milestone

Comments

@SoniEx2
Copy link
Contributor

SoniEx2 commented Oct 24, 2016

I noticed NOVA always uses string identifiers. IMO NOVA should use typed identifiers:

  1. Identifier interface
    • The I at the beginning is necessary to avoid conflicts with a bunch of things (e.g. Kotlin). Could also be NOVAIdentifier - it just can't be Identifier. EDIT: I derped, mixed something up .-.
    • Single method: String asString();
    • Implementations should override equals, hashCode and toString.
  2. UUIDIdentifier class
    • Used when the identifier is an UUID.
    • Additional methods: UUID asUUID();
  3. ClassIdentifier class
    • Used when the identifier is a Class.
    • Additional methods: Class<?> asClass();
  4. StringIdentifier class
    • Used when the identifier is an actual, proper String.

Will make a PR once we figure out if this is a good idea.

@calclavia
Copy link
Member

It's a nice idea, but I feel like it may be an overkill. Can you list out the benefits and drawbacks?

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Oct 25, 2016

As far as I can see right now:

Benefits:

  1. Can downcast
  2. ClassIdentifier compares uniquely for the same name, unlike what we currently have
  3. Can still convert to string when desired
  4. String IDs don't conflict with UUIDs/classnames
  5. Extensible

Drawbacks:

  1. Downcasting needs instanceof checks
  2. Adds more classes

@calclavia
Copy link
Member

I might have misunderstood what you meant by typed identifiers. So you're saying we should extend a base Identifier class and have subclasses that represent different Identifiers... I think that should be fine :)

@ExE-Boss
Copy link
Member

According to the above specification, the base Identifier is supposed to be an interface, but it could also be an abstract class, or it could be an abstract class called AbstractIdentifier implementing the IIdentifier interface.

@ExE-Boss ExE-Boss mentioned this issue Jan 8, 2017
9 tasks
@ExE-Boss ExE-Boss added this to the v0.1.0 milestone Jan 8, 2017
@ExE-Boss ExE-Boss added the breaking change This is going to break stuff label Jan 8, 2017
@RX14 RX14 added the in progress Pull requests that are not yet ready to be merged and issues that are being worked on. label Jan 8, 2017
ExE-Boss added a commit to EB-Archive/NOVA-GUI that referenced this issue Jan 10, 2017
ExE-Boss added a commit that referenced this issue Jan 11, 2017
This reverts commit 5e56559.

# Conflicts:
#	minecraft/1.7/src/main/java/nova/core/wrapper/mc/forge/v17/wrapper/block/BlockConverter.java
#	minecraft/1.7/src/main/java/nova/core/wrapper/mc/forge/v17/wrapper/item/ItemConverter.java
#	minecraft/1.8/src/main/java/nova/core/wrapper/mc/forge/v18/wrapper/block/BlockConverter.java
#	minecraft/1.8/src/main/java/nova/core/wrapper/mc/forge/v18/wrapper/item/ItemConverter.java
#	src/main/java/nova/core/util/registry/Factory.java
ExE-Boss added a commit to EB-Archive/NOVA-Worldgen that referenced this issue Jan 11, 2017
@ExE-Boss ExE-Boss modified the milestones: v0.2.0, v0.1.0 Jan 16, 2017
@ExE-Boss ExE-Boss removed the in progress Pull requests that are not yet ready to be merged and issues that are being worked on. label Oct 21, 2017
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants