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

[json] add asJson as alias for %* #10011

Closed
wants to merge 3 commits into from

Conversation

timotheecour
Copy link
Member

toJson is more readable / searchable / guessable; furthermore easier to use in method call syntax because of required quoting:
foo.toJson vs foo.%*

@andreaferretti
Copy link
Collaborator

andreaferretti commented Dec 16, 2018

Is this

let config = toJson({
  "id": 5000,
  "protocol": "http",
  "timeout": 5.0,
  "params": [1, 2, 3]
})

more readable than this?

let config = %*{
  "id": 5000,
  "protocol": "http",
  "timeout": 5.0,
  "params": [1, 2, 3]
}

I quite like the %* macro - it is not very intrusive visually at it makes writing nested Json structures almost as easy as a dynamic language. toJson feels weirder to me. One reason is that the name makes one think that the thing passed to toJson actually exists as a value, which is then converted to json. So one is left to wonder what is the type of this value, gives that the AST is not well typed. %* is more agnostic and it is easier to think that this is in fact a macro.

@Araq
Copy link
Member

Araq commented Dec 16, 2018

You can easily do

template toJson(x: untyped): JsonNode = json.`%*`(x)

in your own code. But don't make us update our code, %* is fine even though Leibniz didn't know about it.

@Araq Araq closed this Dec 16, 2018
@timotheecour timotheecour reopened this Dec 16, 2018
@timotheecour
Copy link
Member Author

timotheecour commented Dec 16, 2018

  • updated PR to use an alias instead of deprecation so no-one has to update their code in case they prefer the previous name %*
    Having toJson in std/json is consistent with other patterns, eg:
    toBson in nimongo/mongo
    (json is not the only serialization format, eg: protobuf, msgpack, bson, xml etc)

  • I'm happy to change the toJson name exposed in this PR to serializeToJson if you guys prefer (eg as here http://vibed.org/api/vibe.data.json/serializeToJson), or other sensible options that contain the substring json

@andreaferretti

is this...more readable than this...?

yes, IMO it's more readable, making it clear it returns a json

as discussed here: unary operators are often best replaced by explicit names, eg: %* => toJson - Nim forum (which got 6 upvotes)

Nim is not perl, the goal is not to minimize the number of characters we type at the expense of clarity (at least, minimizing the number of tokens we type is a better metric than minimizing the number of characters).

%* is more agnostic and it is easier to think that this is in fact a macro.

I don't quite follow that argument; there lots of macros that are named using regular alphanumeric characters that also take untyped arguments.

@timotheecour timotheecour changed the title add toJson; {.deprecated: [%*: toJson].} [json] add toJson as alias for %* Dec 16, 2018
@vepeckman
Copy link
Contributor

  (json is not the only serialization format, eg: protobuf, msgpack, bson, xml etc)

But it is the most popular, especially in the web development space. It makes sense to make json as frictionless as possible.

as discussed here: unary operators are often best replaced by explicit names, eg: %* => toJson - Nim forum (which got 6 upvotes)

You're taking a couple upvotes on a thread and using that as proof that you're right. 6 upvotes is not consensus that explicit names are better than unary operators in all cases. Plenty of people like %*.

%* is more agnostic and it is easier to think that this is in fact a macro.

I don't quite follow that argument; there lots of macros that are named using regular alphanumeric characters that also take untyped arguments.

The convention for procs named toX is that they take data of one type, and convert it to type X. If I use a proc named toJson, I'm expecting it to take something like a table or string and produce json. Its not clear that toJson is a macro.

I really don't think Nim is any danger of operator abuse. It uses symbols judicially to make common operations shorter, and uses readable names in most cases.

@timotheecour
Copy link
Member Author

The convention for procs named toX is that they take data of one type, and convert it to type X. If I use a proc named toJson, I'm expecting it to take something like a table or string and produce json. Its not clear that toJson is a macro.

that 1 type could be generic though, eg:

# lib/js/jsffi.nim
proc toJs*[T](val: T): JsObject {. importcpp: "(#)" .}
  ## Converts a value of any type to type JsObject

# lib/core/typeinfo.nim:
proc toAny*[T](x: var T): Any {.inline.} =
  ## constructs a ``Any`` object from `x`. This captures `x`'s address, so
# etc (toOpenArray, toFloat...)

or even untyped:

#lib/pure/collections/sequtils.nim:
template toSeq*(iter: untyped): untyped

(although that toSeq example might hopefully one day become template toSeq*(a: typed): untyped if #9374 is implemented)

toSeq (and %*) will work with typed objects (like string or table) but also with untyped ones (to support heterogenous tables/arrays for example), not sure why it's any more confusing than any other macro not starting with to.

It makes sense to make json as frictionless as possible

Thank got we don't have an operator for toSeq even though it's more frequently used than %*. toJson doesn't add any friction.

@GULPF
Copy link
Member

GULPF commented Dec 17, 2018

toJson sounds like a proc that converts its input from one type to another, not a macro that constructs JSON from an AST. The fact that there is another proc that actually do convert its input to JSON (the % operator) which is used by %* makes it even more confusing. People will then wonder, why should I implement % to be able to use toJson for my type?

@timotheecour
Copy link
Member Author

@GULPF I'm fine with other name suggestions, so long they contain the word json
how about:

  • add an alias toJson for %, and add an alias asJson for %*

@GULPF
Copy link
Member

GULPF commented Dec 17, 2018

add an alias toJson for %, and add an alias asJson for %*

@timotheecour toJson/asJson is how it should have been from the beginning IMO, but it might be hard to fix it now. Since %* generates calls to %, supporting an alias for % is messy. What if different modules implement both toJson and % for the same type (doesn't seem extremely unlikely)?

@timotheecour timotheecour changed the title [json] add toJson as alias for %* [json] add asJson as alias for %* Dec 17, 2018
@timotheecour
Copy link
Member Author

timotheecour commented Dec 17, 2018

@timotheecour toJson/asJson is how it should have been from the beginning IMO, but it might be hard to fix it now. Since %* generates calls to %, supporting an alias for % is messy. What if different modules implement both toJson and % for the same type (doesn't seem extremely unlikely)?

all right, I've now used asJson instead of toJson for what %* aliases to; note that the point you're raising only concerns an alias for %, not for %*, so this PR (which only aliases %* to asJson) is ok

Using asJson also address the concern of @nepeckman

The convention for procs named toX is that they [...]

@mratsim
Copy link
Collaborator

mratsim commented Dec 17, 2018

I'm with Timothee, I prefer explicit searchable names over operator unless operators are used pervasively and become a convention.

The Json module should be the example library that shows how a serialisation library should be done and what kind of API can be expected from it.

I would prefer if we standardize on short words like:

macro to*(x: untyped, T: type JsonNode): JsonNode =
  ...
macro as*(x: untyped, T: type JsonNode): JsonNode =
  ...
macro from*(x: JsonNode, T: type Foo): Foo =
  ...
macro parse*(x: JsonNode, T: type Foo): Foo =
  ...

This way YAML, protobuf and even compression libraries can have the same API.
cc @zah as he has a generic serialisation library in mind.

@andreaferretti
Copy link
Collaborator

@timotheecour Let me explain better. If I read let x = something.toJson I am lead to think that - well, something is actually a value.

A common beginner bug in using the json library is transforming something like

let configJson = %*{
  "id": 5000,
  "protocol": "http",
  "timeout": 5.0,
  "params": [1, 2, 3]
}

into the following form

let config = {
  "id": 5000,
  "protocol": "http",
  "timeout": 5.0,
  "params": [1, 2, 3]
}
let configJson = %*config

and then wondering why this does not compile, having "just" moved config into its own variable.

Of course we know that the reason this does not compile is that config in itself is not well typed, and %* is a macro.

Now, I think that if the macro was actually called toJson, the confusion on this point would only increase, since if you convert something to json, then that something must exist in some form before being converted to json. The symbolic notation %*, in my opinion, is more suitable for something that can only be used with literals.

@timotheecour
Copy link
Member Author

timotheecour commented Dec 17, 2018

If I read let x = something.toJson I am lead to think that - well, something is actually a value

@andreaferretti I've already made the change from toJson to asJson to reflect your concern, also raised by @nepeckman above:

The convention for procs named toX is that they take data of one type, and convert it to type X

I think using asJson makes this point clear, and can be used in other similar cases as convention.

The symbolic notation %*, in my opinion, is more suitable for something that can only be used with literals

I can't agree with that because there's more than 1 type that one may convert an AST to. In other industries, protobuf is actually more common (eg, almost everything at google is serialized as protobuf; likewise some domains will deal with Bson, msgpack etc). Since we're gonna have no choice but to use asFoo for type Foo, might as well support asJson for json.

I'm (reluctantly) ok to not deprecate %*, but adding asJson as alias, as this PR does, improves our API.

side note (shouldn't sidetrack this PR)

/cc @mratsim

I would prefer if we standardize on short words like [...]

one issue with macro foo*(x: untyped, T: type JsonNode): JsonNode = is that this works well for typed x, as it can be used in method call syntax (aka UFCS), but not with untyped inputs such as {"name": "foo", "challenge": "bar"}, so it'd force this: foo({"name": "foo", "challenge": "bar"}, JsonNode) which, while good for generic code, is less convenient for non-generic code. Furthermore:

  • as is a keyword so can't be used without quotes, so it's not good
  • parse IMO should only be used for input string (see related https://github.com/nim-lang/Nim/issues/7430), otherwise it'd lead to ambiguity when given input string which could mean either ast or string.
  • to has same issue has others have mentioned, ie, that it's assumed to take a typed input, not an AST

so, if we're also adding a generic alternative as you suggest, it could be instead called parseAst or parseAs and use [] instead of typedesc, eg:

let data = parseAst[JsonNode]({"name": "foo", "challenge": "bar"})

but that would be in addition to the simple asJson, and could be dealt in subsequent PR

@Araq
Copy link
Member

Araq commented Dec 17, 2018

I closed this for a reason. Pointless bikeshedding.

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.

6 participants