String → Identifier #238

ExE-Boss · 2017-01-07T22:47:11Z

@SoniEx2 has pretty much abandoned his PR, so I’m taking over.
For a detailed description of what this does, see #227.

To do list:

Get rid of String serialization.
Redesign this whole thing from scratch. (Seriously, the current implementation is very bloated)
- Get rid of IdentifierFactory (or whatever I called it) and replace it with the Storable interface.

Completed:

Implement StringIdentifier, ClassIdentifier and UUIDIdentifier (SoniEx2)
Implement NamespacedStringIdentifier and EnumIdentifier
- IDs can’t be null. That throws a NullPointerException.
Switch Core to use Identifier everywhere
- Currently kept an implicit String → StringIdentifier method in FactoryManager
  Should we keep it or not?
Write tests
Fix wrappers
- Test in Minecraft 1.7 and 1.8

Closes #227

TODO: Fix wrappers

codecov-io · 2017-01-07T22:50:43Z

Current coverage is 14.26% (diff: 28.48%)

Merging #238 into master will decrease coverage by 0.01%

@@             master       #238   diff @@
==========================================
  Files           393        398     +5   
  Lines         10984      11076    +92   
  Methods           0          0          
  Messages          0          0          
  Branches       1580       1585     +5   
==========================================
+ Hits           1569       1580    +11   
- Misses         9329       9405    +76   
- Partials         86         91     +5

Powered by Codecov. Last update fc4146f...16cf3a9

SoniEx2 · 2017-01-08T04:36:44Z

null IDs?!

You may disagree but I believe this thing should NPE everywhere.

…g and back

SoniEx2 · 2017-01-08T04:53:55Z

You need to learn how to use a damn immutable object.

And a constructor for such an immutable object.

Victorious3 · 2017-01-08T10:25:51Z

Ive yet to understand why you'd need these. Interned strings are enough of an identifier to me...

You know, something like this:

interface Identifier {
    public static String of(String str) { return String.intern(str); }
    public static String of(UUID uuid) { return String.intern(uuid.toString()); }
    public static String of(Class<?> clazz) { return String.intern(clazz.getName()); }
}

Or if you don't like this because of the pitfall that you have to call Identifier.of() to make sure that it is an interned String...

class Identifier {
    private String interned;
    private Identifier(String id) { this.interned = String.intern(id); }

    @Override
    public boolean equals(Object o) {
        // Look at how simple equality becomes
        if (o instanceof Identifier) 
            return ((Identifier)o).interned == this.interned;
        return false;
    }

    @Override
    public int hashCode() { return interned.hashCode(); }

    public static Identifier of(String str) { return new Identifier(str); }
    public static Identifier of(UUID uuid) { return new Identifier(uuid.toString()); }
    public static Identifier of(Class<?> clazz) { return new Identifier(clazz.getName()); }
}

Downside of that is that you can't compare them with == but that's otherwise "normal" in java anyways. Forget about all of that "serializing back to specific Identifier type" crud. Calling Class.forName or UUID.fromString respectively should be enough to expect of someone to do, if really needed. Not once have I seen such a case so far. What you do here is excessive over-engineering.

RX14 · 2017-01-08T10:18:47Z

src/main/java/nova/core/util/id/AbstractIdentifier.java

+	@Override
+	public abstract boolean equals(Object other);
+
+	protected static final <T extends Identifier> boolean equalsImpl(Identifier _this, Object other, Class<T> superclass, Function<T,Object> getter) {


self instead of _this? Is there a convention on this like clazz?

RX14 · 2017-01-08T10:42:16Z

src/test/java/nova/core/util/EnumExample.java

+/**
+ * @author ExE Boss
+ */
+public enum EnumExample {


This change shouldn't be in this PR.

This change was done to allow testing of EnumIdentifier using my EnumExample class.

RX14 · 2017-01-08T11:26:49Z

@Victorious3 wouldn't interned strings technically work with ==? I wouldn't recommend it because it might be a bit flakey but if they are interned shouldn't they be the "same" string.

…s in base identifier types final

Victorious3 · 2017-01-08T12:02:12Z

@RX14 Yes they do see my 0815 implementation. Hence why I said interned strings are all you need for identifers. And

Identifier.of("foo") == Identifier.of(new String(new char[] {'f', 'o', 'o'})));

Not if you go with the simple wrapper class approach tho. Which is more safe because you wont accidentally forget to call Identifier.of somewhere

SoniEx2 · 2017-01-08T14:16:20Z

@Victorious3 The issue is Identifier.of("net.minecraft.block.Block") == Identifier.of(net.minecraft.block.Block.class). That should NEVER be able to happen.

BUT OTHER THAN THAT, THEY SHOULD ACT LIKE THE String CLASS: THEY SHOULD BE IMMUTABLE. Imagine you have an Identifier in 20 different places, including hashmap keys and stuff, then you accidentally call .load() on it. For one it'd break the hashmap contract. And it's just plain insane!

@ExE-Boss Could you at least TALK TO ME ABOUT IT before doing something completely messed up?!

RX14 · 2017-01-08T14:34:58Z

@SoniEx2 How about you calm the fuck down and suggest improvements nicely instead of shouting at people. It won't make them take your input more seriously.

SoniEx2 · 2017-01-08T15:05:29Z

@RX14 I'll happily shout at someone who's touching code I wrote without any idea of what the code should do.

RX14 · 2017-01-08T15:07:05Z

@SoniEx2 Well, if you keep doing it you won't be commenting on this issue tracker any more. You talk nicely or don't talk at all.

SoniEx2 · 2017-01-08T15:16:44Z

@RX14 Fine.

Part of the reason I made the PR was so I could put different kinds of identifiers in the same HashMap without having them conflict. Something I couldn't do with strings.

A NamespacedStringIdentifier should contain an inner static class called NamespacedStringData, and that class should be used as the identifier type. The equals and hashCode methods should be final, since otherwise equals wouldn't be symmetric. This is also why we can't/shouldn't have null IDs.

SoniEx2 · 2017-01-08T17:30:49Z

And string is useless for older Minecraft versions.

Victorious3 · 2017-01-08T17:32:29Z

@SoniEx2 This is Nova CORE it doesn't CARE about "Minecraft Versions"

SoniEx2 · 2017-01-08T17:34:22Z

And the Minecraft wrappers are directly integrated into NOVA Core (kinda why they're in the same repo instead of being in their own repos).

Victorious3 · 2017-01-08T17:36:31Z

@SoniEx2 If you manage to name one problem that this overbloated PR supposedly solves, then go for it. You are shooting with cannons at ants. I'm for simplicity and ease of use and things like NamespacedStringIdentifier certainly don`t fall into that category.

SoniEx2 · 2017-01-08T17:37:22Z

Please make Mojang remove Minecraft's ResourceLocation. It's bloated.

Victorious3 · 2017-01-08T17:50:12Z

@SoniEx2 Besides this being completely off point, ResourceLocation is NOT an identifier. It serves the purpose of being able to locate files within the jar or resource packs which use completely different file paths.

SoniEx2 · 2017-01-08T17:51:13Z

@Victorious3 It is actually NO different from a NamespacedStringIdentifier. It identifies block names, item names, etc.

Victorious3 · 2017-01-08T17:54:33Z

@SoniEx2 You are impossible to talk to. Closed until further notice from @ExE-Boss

RX14 · 2017-01-08T17:56:49Z

@Victorious3 While I share your opinion, that doesn't mean I won't remove you from the project or issue tracker if you continue to be rude.

SoniEx2 · 2017-01-08T17:59:28Z

You just need to talk to some java pros about it. Maybe even @calclavia (altho I'm not sure how "pro" they are).

ExE-Boss · 2017-01-08T18:01:53Z

On the NOVA discord server, I’ve discussed this with RX14 and we decided the following:

I would continue working on my version of the Identifier API and once all the versions of the Identifier API are fully implemented, we will compare them and decide which implementation is the best and end up using that.

SoniEx2 · 2017-01-08T18:03:36Z

I need something I can use with HashMaps and stuff. I cannot use this implementation, and strings are unsuitable due to their conflicting nature.

Victorious3 · 2017-01-08T18:04:14Z

@SoniEx2 You dont throw two different types of Objects into the same map. Just dont do it. Then you are fine.

SoniEx2 · 2017-01-08T18:39:23Z

I can throw String and Class and UUID all into the same Map just fine. They don't conflict with eachother.

I want it to also work for Identifiers.

It has its uses.

(Also, part of the reason I made the PR was so mods could extend Identifier and create their own Identifiers for their own purposes. Like using Identifier for a programming language/compiler. That is where being able to mix Identifiers together in a Map is a really useful feature to have. I agree that @ExE-Boss 's version is bloated. It's also unsuitable for my use-case. Unlike my version.)

Victorious3 · 2017-01-08T23:13:05Z

I can throw String and Class and UUID all into the same Map just fine. They don't conflict with eachother.

And then its a Map<Object, Object> hell no.

Like using Identifier for a programming language/compiler.

Mhm. Sounds like something I'd do every day.

loordgek · 2017-01-15T00:16:21Z

@Victorious3 ResourceLocation IS used as a identifier https://github.com/MinecraftForge/MinecraftForge/blob/1.11.x/src/main/java/net/minecraftforge/event/AttachCapabilitiesEvent.java#L74
https://github.com/MinecraftForge/MinecraftForge/blob/1.11.x/src/main/java/net/minecraftforge/fml/common/registry/IForgeRegistryEntry.java#L50

Victorious3 · 2017-01-15T00:26:29Z

@loordgek And so might any other object that implements hashcode and equals. Yet you wouldn't call them identifiers.

SoniEx2 · 2017-01-15T00:27:58Z

I miss @calclavia, the only professional/sane java dev we had on this POS.

loordgek · 2017-01-15T00:29:05Z

@SoniEx2 can you stop crying ranting plz

calclavia · 2017-01-15T00:33:06Z

@ExE-Boss What's the NOVA discord server? I'd like to join just to see what you guys are up to.

ExE-Boss · 2017-01-15T07:07:07Z

@calclavia The server is https://discord.gg/Sh63S4H

SoniEx2 · 2017-01-15T12:01:21Z

@loordgek Never. If you don't like it, why do you keep doing things that cause it?

RX14 · 2017-01-15T12:23:40Z

@SoniEx2 goodbye

SoniEx2 and others added 5 commits October 23, 2016 18:32

Remove BlockProperties

70493f6

String -> Identifier. WIP.

5e56559

Merge branch 'master' into refact-2

cf56700

Work on Identifiers

04e3ae2

Change all instances of String id to Identifier in core

f773b01

TODO: Fix wrappers

ExE-Boss added breaking change This is going to break stuff in progress Pull requests that are not yet ready to be merged and issues that are being worked on. labels Jan 7, 2017

ExE-Boss added this to the v0.1.0 milestone Jan 7, 2017

ExE-Boss self-assigned this Jan 7, 2017

ExE-Boss added 2 commits January 8, 2017 02:52

Write tests + Fix Identifier.equals + Create EnumIdentifier

3954e02

Make it possible to store Identifiers in Data + Write tests

9e3d76a

ExE-Boss mentioned this pull request Jan 8, 2017

String -> Identifier. WIP. #229

Closed

ExE-Boss added 2 commits January 8, 2017 05:38

Improve the way Identifiers are stored + Allow serialization to strin…

fe9c0a9

…g and back

Fix Travis build

6d8c194

Fix wrappers + fluid id saving

daad474

ExE-Boss requested a review from RX14 January 8, 2017 05:33

RX14 suggested changes Jan 8, 2017

View reviewed changes

ExE-Boss added 2 commits January 8, 2017 12:58

IDs must be nonnull + rename _this to self in equalsImpl + make equal…

fca4249

…s in base identifier types final

Merge branch 'master' into identifier

16cf3a9

Victorious3 closed this Jan 8, 2017

RX14 removed the in progress Pull requests that are not yet ready to be merged and issues that are being worked on. label Jan 8, 2017

RX14 reopened this Jan 8, 2017

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 removed this from the v0.1.0 milestone Jan 8, 2017

Merge branch 'master' into identifier

25e7a65

RX14 closed this Feb 24, 2017

String → Identifier #238

String → Identifier #238

Conversation

ExE-Boss commented Jan 7, 2017 • edited Loading

To do list:

Completed:

codecov-io commented Jan 7, 2017 • edited Loading

Current coverage is 14.26% (diff: 28.48%)

SoniEx2 commented Jan 8, 2017 • edited Loading

SoniEx2 commented Jan 8, 2017

Victorious3 commented Jan 8, 2017 • edited Loading

RX14 Jan 8, 2017

Choose a reason for hiding this comment

ExE-Boss Jan 8, 2017

Choose a reason for hiding this comment

RX14 Jan 8, 2017

Choose a reason for hiding this comment

ExE-Boss Jan 8, 2017

Choose a reason for hiding this comment

RX14 Jan 8, 2017

Choose a reason for hiding this comment

RX14 commented Jan 8, 2017

Victorious3 commented Jan 8, 2017 • edited Loading

SoniEx2 commented Jan 8, 2017 • edited Loading

RX14 commented Jan 8, 2017

SoniEx2 commented Jan 8, 2017

RX14 commented Jan 8, 2017 • edited Loading

SoniEx2 commented Jan 8, 2017 • edited Loading

SoniEx2 commented Jan 8, 2017

Victorious3 commented Jan 8, 2017

SoniEx2 commented Jan 8, 2017

Victorious3 commented Jan 8, 2017

SoniEx2 commented Jan 8, 2017

Victorious3 commented Jan 8, 2017

SoniEx2 commented Jan 8, 2017

Victorious3 commented Jan 8, 2017

RX14 commented Jan 8, 2017

SoniEx2 commented Jan 8, 2017

ExE-Boss commented Jan 8, 2017 • edited Loading

SoniEx2 commented Jan 8, 2017

Victorious3 commented Jan 8, 2017

SoniEx2 commented Jan 8, 2017 • edited Loading

Victorious3 commented Jan 8, 2017 • edited Loading

loordgek commented Jan 15, 2017

Victorious3 commented Jan 15, 2017

SoniEx2 commented Jan 15, 2017

loordgek commented Jan 15, 2017

calclavia commented Jan 15, 2017

ExE-Boss commented Jan 15, 2017

SoniEx2 commented Jan 15, 2017

RX14 commented Jan 15, 2017

ExE-Boss commented Jan 7, 2017 •

edited

Loading

codecov-io commented Jan 7, 2017 •

edited

Loading

SoniEx2 commented Jan 8, 2017 •

edited

Loading

Victorious3 commented Jan 8, 2017 •

edited

Loading

Victorious3 commented Jan 8, 2017 •

edited

Loading

SoniEx2 commented Jan 8, 2017 •

edited

Loading

RX14 commented Jan 8, 2017 •

edited

Loading

SoniEx2 commented Jan 8, 2017 •

edited

Loading

ExE-Boss commented Jan 8, 2017 •

edited

Loading

SoniEx2 commented Jan 8, 2017 •

edited

Loading

Victorious3 commented Jan 8, 2017 •

edited

Loading