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

Allow using IndexMap to preserve fields order #784

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

juchiast
Copy link

No description provided.

@schungx
Copy link
Collaborator

schungx commented Nov 26, 2023

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...

@juchiast
Copy link
Author

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

@schungx
Copy link
Collaborator

schungx commented Nov 26, 2023

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?

@juchiast
Copy link
Author

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
Copy link
Author

juchiast commented Nov 26, 2023

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

@schungx
Copy link
Collaborator

schungx commented Nov 26, 2023

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...

@juchiast
Copy link
Author

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.

@@ -486,6 +487,18 @@ impl Expr {
Dynamic::from_map(map)
}

#[cfg(not(feature = "no_object"))]
#[cfg(feature = "indexmap")]
Self::Map(x, ..) if self.is_constant() => {
Copy link
Author

@juchiast juchiast Nov 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and in eval.rs, the code use BTreeMap directly instead of crate::Map, I don't know if changing it would have any side-effects, so I just add new code for indexmap feature.

@schungx
Copy link
Collaborator

schungx commented Nov 26, 2023

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...

@h7kanna
Copy link

h7kanna commented Dec 21, 2023

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

@schungx
Copy link
Collaborator

schungx commented Dec 21, 2023

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants