while this code compiles fine
from strutils import `%`
type
    Roles = enum
        user,catering,manager,sysadmin

converter toRole(s : string): Roles =
    case s
        of "user":
            return Roles.user
        of "catering":
            return Roles.catering
        of "manager":
            return Roles.manager
        of "sysadmin":
            return Roles.sysadmin
        else:
            raise newException(ValueError, "could not convert string '$#' to Roles" % [s])
this one does not
from strutils import `%`
type
    Roles = enum
        user,catering,manager,sysadmin

converter toRole(s : string): Roles =
    case s
        of "user":
            Roles.user
        of "catering":
            Roles.catering
        of "manager":
            Roles.manager
        of "sysadmin":
            Roles.sysadmin
        else:
            raise newException(ValueError, "could not convert string '$#' to Roles" % [s])
while this also compiles
from strutils import `%`
type
    Roles = enum
        user,catering,manager,sysadmin

converter toRole(s : string): Roles =
    case s
        of "user":
            Roles.user
        of "catering":
            Roles.catering
        of "manager":
            Roles.manager
        of "sysadmin":
            Roles.sysadmin
        else:
            Roles.sysadmin
Isn't it bad to force to write the return explicitly only because one branch of the case does not return?
2017-11-10 20:33:24

(slightly off topic by not addressing directly the case statement, but I think this is the heart of the issue)

If there was an "Invalid" role, which is the "else" case,

type
    Roles = enum
        invalid,user,catering,manager,sysadmin
then the consumer of this converter could test and handle that themselves via the "role" rather than via exception handling.

But which approach is the preferred approach?

2017-11-10 21:30:56

(heavily off topic, just matter of personale taste)

I suspected this would have been the proposed solution

I started to appreciate exceptions in my Java past, and I still use them in C++ even though my peers seem not to appreciate much having to enclose the opening of a file in a try block.. Instead I really don't like having to check a return code or the validity o a return value (like in this case): it breaks the natural flow of the code (while you read it, obviously when the code runs exceptions have the same effect) and I especially hate when you have to check the same thing multiple times.

e.g:

In C when you want to read a file:

  • you have to check the return value of the fopen function because it can break if the file doesn't exist (even though, on linux, it doesn't break when you pass to it a directory path)
  • you have to check the return value of the fread function because it can break for whatever reason (one randomly chosen: you are reading a directory on linux)
  • you have to check the return value of the fclose function because it can break if the disk is full

How much better is it to just wrap everything in a try/catch(IOException) block and forget about details?

2017-11-10 22:29:44

I would say: it depends. If you have hard time and/or memory constraints don´t use exceptions (small memory footprint embedded stuff). Besides of that I would prefer using exceptions. But do not misuse exceptions (i.e. handling state) and/ or catch it to early. If you catch it but you need a raise you should check your design . And never ever swallow it silently..

In the specific example I (personally) prefer the invalid-role variant. After returning from toRole you can throw the exception there (but also with the given (invalid) role-input-string.

EDIT (I missed the conclusion): This has one advantage: the converter just converts and the interpretation (of the conversion) comes at a higher level; so your conversion is atomic which is preferable. You can see the exception as an extra return value but a (truly atomic) converter shouldn´t do two things (convert and interpret) so it´s better to map the unknown input value to the special "unknown"-code.

But don´t map the unkown input value to a sysadmin-role - that would be a very nice security hole :->

2017-11-11 00:07:07

The fact is that, when I convert a string to an enum it is typically because I'm deserializing a value that comes from a file my program previously wrote or from the network because I received it from a webservice (which is the case of this example), I'm not reading user input, so the else case doesn't actually ever happen unless I have a huge bug in my code, in which case I'm perfectly fine with the whole process to crash XD

Only when I really have to validate the string (e.g. user typed the value in a text field) I need to enclose the conversion in a try block

2017-11-11 08:34:28

btw, since I don't want to write the return myself, I persuaded the compiler to do it for me

import macros
from strutils import `%`
macro Enum*(typeName, body) : untyped =
    let enumVar = newIdentNode("e")
    let strVar = newIdentNode("s")
    
    var caseConverterStatement = nnkCaseStmt.newTree(
        strVar
    )
    
    let converterDecl = nnkConverterDef.newTree(
        newIdentNode("to" & $typename.ident),
        newEmptyNode(),
        newEmptyNode(),
        nnkFormalParams.newTree(
            typename,
            nnkIdentDefs.newTree(
                strVar,
                newIdentNode("string"),
                newEmptyNode()
            )
        ),
        newEmptyNode(),
        newEmptyNode(),
        nnkstmtList.newTree(
            caseConverterStatement
        )
    )
    
    let converterRaise = nnkElse.newTree(
        nnkstmtList.newTree(
            nnkRaiseStmt.newTree(
                nnkCall.newTree(
                    newIdentNode("newException"),
                    newIdentNode("ValueError"),
                    nnkInfix.newTree(
                        bindSym("%"),
                        newStrLitNode("could not convert string '$#' to " & $typename.ident),
                        nnkBracket.newTree(
                            strVar
                        )
                    )
                )
            )
        )
    )
    let makeConverterBranch = proc(typeName, enumName : NimNode, enumStr = newEmptyNode()) : NimNode =
        nnkOfbranch.newtree(
            if enumStr.kind() == nnkStrLit: enumStr  else: newStrLitNode($enumName.ident),
            nnkstmtList.newTree(
                nnkReturnStmt.newTree(
                    nnkDotExpr.newTree(
                        typeName,
                        enumName
                    )
                )
            )
        )
    
    
    let makeDollarBranch = proc(typeName, enumName : NimNode, enumStr = newEmptyNode()) : NimNode =
        nnkOfbranch.newtree(
            nnkDotExpr.newTree(
                typename,
                enumName
            ),
            nnkstmtList.newTree(
                if enumStr.kind() == nnkStrLit: enumStr  else: newStrLitNode($enumName.ident)
            )
        )
    var caseDollarStatement = nnkCaseStmt.newTree(
        enumVar
    )
    let dollarDecl = nnkProcDef.newTree(
        nnkAccQuoted.newTree(
            newIdentNode("$"),
        ),
        newEmptyNode(),
        newEmptyNode(),
        nnkFormalParams.newTree(
            newIdentNode("string"),
            nnkIdentDefs.newtree(
                enumVar,
                typeName,
                newEmptyNode()
            )
        ),
        newEmptyNode(),
        newEmptyNode(),
        nnkstmtList.newTree(
            caseDollarStatement
        )
    )
    
    var enumNames = nnkEnumTy.newTree(newEmptyNode())
    let enumDecl = nnkTypeSection.newtree(
        nnkTypeDef.newtree(
            typeName, newEmptyNode(), enumNames
        )
    )
    for child in body:
        case child.kind:
            of nnkIdent:
                enumNames.add(child)
                caseDollarStatement.add(makeDollarBranch(typeName, child))
                caseConverterStatement.add(makeConverterBranch(typeName,child))
            of nnkAsgn:
                enumNames.add(child[0])
                caseDollarStatement.add(makeDollarBranch(typeName, child[0], child[1]))
                caseConverterStatement.add(makeConverterBranch(typeName,child[0], child[1]))
            else:
                discard
    caseConverterStatement.add(converterRaise)
    result = nnkstmtList.newTree(
        enumDecl,
        dollarDecl,
        converterDecl
    )

################# Usage #########################

Enum(Roles):
    user; manager
    catering = "whatever"
    sysadmin

block:
    let user = Roles.user
    let manager = Roles.manager
    let catering = Roles.catering
    
    echo user
    echo manager
    echo catering
    
    var u = Roles("user")
    echo u == Roles.user
this language is damn great...

2017-11-11 08:54:18
In my opinion it should be treated like a bug. Looks close to this one https://github.com/nim-lang/Nim/issues/6609
2017-11-11 10:12:14
Call me a weirdo but I think a converter which could fail should return an option[T] instead of T itself. This way you can either unwrap the option (works like exception) or pass it somewhere first, e.g. into a container so that you could use unwrap inside of an higher-order routine.
2017-11-11 23:47:57