ldtteam / Structurize

Minecraft Structures
GNU General Public License v3.0
44 stars 45 forks source link

easier scan name, allow double for version (handle double internally) #662

Closed Raycoms closed 4 months ago

Raycoms commented 4 months ago

Closes # Closes # Closes #

Changes proposed in this pull request:

Review please

Raycoms commented 4 months ago

Why double? doesn't seem the best for versioning - or is it only for visual things?

1.0 1.1 1.2 1.3 instead of only 1 2 3 4

Nightenom commented 4 months ago

ooof, imho use sth better than double, anything like record, artifact version, or idk what's available double is based on locale etc.

uecasm commented 4 months ago

ArtifactVersion sounds like a probably good choice. The parser could even be smart enough to support both plain int and stringified artifact version, for backwards compatibility.

Raycoms commented 4 months ago

ArtifactVersion sounds like a probably good choice. The parser could even be smart enough to support both plain int and stringified artifact version, for backwards compatibility.

The question is pretty much. Do we need it? Double supports both an int and a double (e.g. 1 and 1.0 and 1.2 and 2,3,4).

Artifact version supports that too, but in this context is an overkill compared to what we need. Like, int versions are technically enough, but are a bit boring if you want to show the progress properly.

uecasm commented 4 months ago

If you had to write your own string parser, I'd say no. Since there's one you can use for free, I say why not? 😀

With a full artifact version it might even make sense to display it (in tiny font) on the style pack selector, so people can see if they have a BETA pack. Or you could get a more descriptive warning message if there's a mismatch between client/server.

double is based on locale etc.

Though fwiw that's not really a problem here. The JSON parse is locale-independent and currently it's not displayed anywhere, so there's no other locale-related problems. There might be precision-related problems, but that's unlikely to be an issue in practice.