rhai icon indicating copy to clipboard operation
rhai copied to clipboard

Allow using IndexMap to preserve fields order

Open juchiast opened this issue 8 months ago • 10 comments

juchiast avatar Nov 25 '23 17:11 juchiast

I am not sure how useful this feature would be in general... AsIndexMap may be only one of a number of variants that users may want.

If we open this up, then some users may want other types as well.

Maybe it is possible to put a generic trait on Dynamic and Engine but that would be a big change...

I would much rather think of a different way to achieve your purpose...

schungx avatar Nov 26 '23 01:11 schungx

Yeah we need the value returned by #{ } syntax to preserve fields order and this seems like the simplest way to achieve this.

juchiast avatar Nov 26 '23 02:11 juchiast

Is there any particular reason why you'd need to preserve order?

For display purposes? Or to compare two maps by comparing their textual representation?

schungx avatar Nov 26 '23 05:11 schungx

For display purposes, we use RHAI for scripting in our site, one of the task is to build config files, so it is preferable to keep the fields order.

juchiast avatar Nov 26 '23 05:11 juchiast

Yeah we need the value returned by #{ } syntax to preserve fields order and this seems like the simplest way to achieve this.

Additionally, maps also need to preserve order after running a script, so that we can use RHAI to update config like this

x["name"] = "Foo";
x

juchiast avatar Nov 26 '23 05:11 juchiast

Yes, I can see how preserving order would be useful for a config file...

However, it seems that IndexMap has the exact API as HashMap, so wouldn't it be a simple matter to redefine the Map type without changing any code at all?

In that case we might use build logic to actually override the type for Map...

schungx avatar Nov 26 '23 06:11 schungx

However, it seems that IndexMap has the exact API as HashMap, so wouldn't it be a simple matter to redefine the Map type without changing any code at all?

There are some differences to BTreeMap, the one you are using, such as no Hash implementation on IndexMap.

juchiast avatar Nov 26 '23 06:11 juchiast

Ah, alright. I forgot that Map is a BTreeMap. Yes there are some API differences.

I understand your intended usage but still it seems a niche requirement that may not call for adding a feature...

schungx avatar Nov 26 '23 10:11 schungx

There is a precedence of adding a feature of 'preserve_order' in other crates for example (incase you want to consider adding this feature) serde : https://github.com/serde-rs/json/blob/master/Cargo.toml#L58 schemars : https://github.com/GREsau/schemars/blob/master/schemars/Cargo.toml

h7kanna avatar Dec 21 '23 02:12 h7kanna

I understand your point, but the fact is that an early decision was made too make the Map data type a simple type-def to BTreeMap<SmartString, Dynamic>... changing that would necessarily break some user code when they depend on the BTreeMap API, unless that substitute is an exact drop-in replacement for BTreeMap...

But again, I would consider changing Map to point to IndexMap under a feature gate...

schungx avatar Dec 21 '23 06:12 schungx