Skip to content

Conversation

@lemz1
Copy link

@lemz1 lemz1 commented Apr 26, 2025

LINKED ISSUE

FunkinCrew/Funkin#4793

DESCRIPTION

This pr adds the :order metadata for maps. It accepts an array of keys. The writer exports the values in the order of the keys. Any key that isn't in the :order metadata will be appended in alphabetical or ascending order (depends on what Reflect.compare does for the type).

When the value is equal to :default then it is skipped when exporting.

EXAMPLE

class Data
{
	@:order(["easy", "normal", "hard"])
	public var scores:Map<String, Int>;
        // example
        // keys: "easy", "expert", "nightmare", "normal", "hard"
        // output: "easy", "normal", "hard", "expert", "nightmare"

	@:order([1, 2, 3])
	public var values:Map<Int, Int>;
        // example
        // keys: 1, 3, 5, 2, 4
        // output: 1, 2, 3, 4, 5
}

@lemz1 lemz1 marked this pull request as draft April 26, 2025 14:16
@lemz1 lemz1 force-pushed the feature/ordered-map-keys branch from 586257c to 94d9a34 Compare April 26, 2025 14:51
@lemz1 lemz1 marked this pull request as ready for review April 26, 2025 14:52
@EliteMasterEric
Copy link
Member

Wait this has been around for FOUR MONTHS and nobody told me? 😱

@EliteMasterEric
Copy link
Member

My excuse for not reviewing last week is that I was sick.

@EliteMasterEric
Copy link
Member

My notes so far:

  • The macro broke on StageData, it didn't like trying to compare to an enum with an argument, like haxe.ds.Either.Left(value). I modified it to never skip if the value of @:default is a function call expression (since technically an enum with an argument is a function call?).
  • Ignoring default values should probably be a parameter, like ignoreNullOptionals. And honestly, both should be specified via a parameter object instead of ordered constructor arguments. That can be done in another PR though.
  • Ignoring default values doesn't work on arrays. This is because equality for anything other than primitives is based on identity, not value. We'd need to either implement or import an implementation of deep equality to handle arrays like this.

Before:
image
After:
image

@EliteMasterEric
Copy link
Member

Would still like to find a nice fix for arrays before merging.

@lemz1
Copy link
Author

lemz1 commented Aug 13, 2025

im gonna look into it, and see if there is a good solution

@lemz1
Copy link
Author

lemz1 commented Aug 15, 2025

arrays should work now

@EliteMasterEric
Copy link
Member

Just got around to trying this and it looks like this breaks because @:default([]) is sometimes a Map<K, V>() and when it is, field.length doesn't exist.

Definitely try building and testing this with the Funkin' repo (since that's the main place we want to use these changes).

@lemz1
Copy link
Author

lemz1 commented Sep 6, 2025

Just got around to trying this and it looks like this breaks because @:default([]) is sometimes a Map<K, V>() and when it is, field.length doesn't exist.

Definitely try building and testing this with the Funkin' repo (since that's the main place we want to use these changes).

i got maps to compile now, i haven't got around to testing yet though.
so i am not sure whether it actually works or not.
but at the very least it compiles now.

@EliteMasterEric
Copy link
Member

This outputs data that's oddly compressed?

image

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.

2 participants