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

Support only some types as JsAssoc keys, fix #8601 #8627

Merged
merged 1 commit into from
Nov 23, 2018

Conversation

alehander92
Copy link
Contributor

We limit JsAssoc keys to types with values that can be mapped 1 to 1 with cstring .
Drop the support for JsAssoc[string, V] . Still, one can do JsAssoc[cstring, string]{a: "e", "b": "e"} and access cstring keys with string literals.
Convert keys automatically in iterators

@alehander92
Copy link
Contributor Author

alehander92 commented Aug 13, 2018

One can still support any type as a key, if he adds a function for it that matches the concept.
We can name this jshash or something, but I am not sure if that would be correct

@Araq
Copy link
Member

Araq commented Aug 13, 2018

I don't like it. JsAssoc should only support cstring as the key type, no concept mumbo jumbo, no integers that are converted to strings under-the-hood.

@alehander92
Copy link
Contributor Author

I don't agree, it's straightforward to support integers/enums(as far as I understand, floats too) in a type safe way and there is one obvious way to convert them: if JsAssoc doesn't support that, everyone will just write their own custom incompatible JsAssoc alternative (I would).

The problem with other types is that there isn't an obvious way to map it correctly, so it doesn't make sense to provide a stdlib default.

I thought concepts are already fine for the stdlib?

Copy link
Member

@Araq Araq left a comment

Choose a reason for hiding this comment

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

At least give it better names.

lib/js/jsffi.nim Outdated
@@ -69,10 +69,25 @@ template mangleJsName(name: cstring): cstring =
inc nameCounter
"mangledName" & $nameCounter

# only values that can be mapped 1 to 1 with cstring should be keys: they have an injective function with cstring

proc injectiveTo*[T: SomeInteger](text: cstring, t: type T): T {.importcpp: "parseInt(#)".}
Copy link
Member

Choose a reason for hiding this comment

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

name these procs toJsKey

lib/js/jsffi.nim Outdated
type
JsInjective* = concept a, type T
Copy link
Member

Choose a reason for hiding this comment

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

JsKey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

lib/js/jsffi.nim Outdated
{. importcpp: setImpl .}
## Set the value of a property of name `field` in a JsAssoc `obj` to `v`.

proc `[]=`*[V](obj: JsAssoc[string, V], field: cstring, val: V)
Copy link
Member

Choose a reason for hiding this comment

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

Why was it written this way? JsAssoc only works with cstring, not with string.

@alehander92
Copy link
Contributor Author

unrelated fail, comment addressed

@Araq Araq merged commit 88d707c into nim-lang:devel Nov 23, 2018
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.

None yet

2 participants