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

[WIP] first pass at fixing #7430: universal to conversion template #7488

Closed
wants to merge 1 commit into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Apr 3, 2018

here's a 1st pass at https://github.com/nim-lang/Nim/issues/7430 ; I (or others) can add more conversions once we agree on design principles below

design principles:

  • each to overload should be in the module where the types / conversions are defined, eg s.to(JsonNode) goes in lib/pure/json.nim , s.to(bool) goes in lib/pure/strutils.nim etc

this is preferable to having a dedicated hodge-podge conv.nim module that would import everything needed and wrap for eg parseFoo to to(Foo) ; that would not scale (eg wouldn't work with nimble packages, which can't be imported by conv.nim) ;

  • the existing parseFoo(a:string) toFoo(...) etc should become thin wrappers around the corresponding to proc/template (the to would contain the actual implementation) and eventually deprecated
    eg: see this PR for parseBool becoming thin wrapper around s.to(bool)

  • prefer proc to template whenever possible

  • we should standardize ASAP on ideal signature, eg the name of arguments matters in case generic code uses named parameter syntax (eg: to(s:foo, Bar); so I suggest:

proc to*(s: string; dest: typedesc[T]): T = ...

which is short and generic (s standing for source)

  • new conversion functions (eg what would be currently called parseFoo or toFoo) should just use to and not even define parseFoo or toFoo

@timotheecour timotheecour changed the title address #7430: to first pass at fixing #7430: universal to conversion template Apr 3, 2018
@timotheecour timotheecour changed the title first pass at fixing #7430: universal to conversion template [WIP] first pass at fixing #7430: universal to conversion template Apr 3, 2018
@Araq
Copy link
Member

Araq commented Apr 4, 2018

I think to should really be named parse in order to better convey what it does. It also makes it more obvious that parsing can fail and it would raise an exception then.

@PMunch
Copy link
Contributor

PMunch commented Apr 4, 2018

I think the idea was to not only support parsing, but general conversion though

@Araq
Copy link
Member

Araq commented Apr 4, 2018

Meh, general conversion is too general. Think about error handling/reporting.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 4, 2018

@Araq how about:
each appropriate module defines overloaded parse procs/template: eg:

# only for parsing strings
proc parse(s: string; dest: typedesc[bool]): bool = s.parseBool
echo "true".parse(bool)
# for other conversions we have `to` conversion procs/templates defined in their own modules, for eg
var a:A
echo a.to(B)

somewhere (eg in conv.nim or strutils.nim) we also add:

# most generic
template to*[T](s: string; dest: typedesc[T]): auto = s.parse(dest)
# still works
echo "true".to(bool)

that way user can choose the most generic to or the string specific parse depending on his use case;
all the parse overloads conform to a given signature (feel free to suggest improvements)

Think about error handling/reporting.

can you elaborate what you'd want for that?

@dom96
Copy link
Contributor

dom96 commented Apr 4, 2018

I agree with @Araq.

Think about the following:

"tru".to(bool) # Error? False?
"{{{".to(JsonNode) # Definitely an error.

parse is a better name here. The to name implies a strict conversion that has some compile-time checks to me.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 4, 2018

I think the semantics should be obvious here:

"tru".to(bool) # throws Exception (this is not javascript where everything unsafely converts to everything ;-) )
"{{{".to(JsonNode) # throws Exception

but again, see the proposal for having to + parse in #7488 (comment) which IMO addresses all your concerns

The to name implies a strict conversion that has some compile-time checks to me

can you elaborate here (say w an example)? the input argument is not known at compile time, so we'd get RT errors when conversion fails

I think to should really be named parse in order to better convey what it does. It also makes it more obvious that parsing can fail and it would raise an exception then.

other conversions not involving input string can also raise an exception, eg:

257.to(uint8) # throws Exception (same as D: rdmd --eval='257.to!(ubyte).writeln;' std/conv.d(1449): Conversion positive overflow
classA_instance.to(classB) # throws Exception (depending on typeid(classA_instance) and it's relation to classB)
jsonNode.to(NimType) # throws Exception if can't be deserialized from jsonNode
protoMsg.to(NimType) # throws Exception if can't be deserialized from protoMsg

likewise with user defined types where it would make sense to throw

@Araq
Copy link
Member

Araq commented Apr 4, 2018

I don't see the need for to, to is an overly generic preposition, Ruby uses it for counting iirc, parse is a verb, procs should be verbs.

@dom96
Copy link
Contributor

dom96 commented Apr 4, 2018

I think until we can ascertain how useful this is in the wild, this should go into a Nimble package. Further down the line we can decide whether it belongs in the stdlib once we see how useful it is.

The stdlib is a gentle thing, we want to be conservative, it's much easier to add things to it than it is to remove.

@PMunch
Copy link
Contributor

PMunch commented Apr 4, 2018

I agree. But I still think we could implement the parse proc in strutils for a more generic interface for parsing. To in Nim can often be done just by the simple 42.uint8; 1.bool, so I don't entirely see the need for that.

@Araq
Copy link
Member

Araq commented Apr 6, 2018

Adding parse support in the stdlib is fine once the "typedesc vs generic parameter" fight is settled though.

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.

4 participants