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

rst.whichMsgClass is buggy: relies on $enum instead of $ast(enum) #17280

Closed
timotheecour opened this issue Mar 7, 2021 · 2 comments · Fixed by #17338
Closed

rst.whichMsgClass is buggy: relies on $enum instead of $ast(enum) #17280

timotheecour opened this issue Mar 7, 2021 · 2 comments · Fixed by #17338

Comments

@timotheecour
Copy link
Member

timotheecour commented Mar 7, 2021

/cc @a-mr
root cause for #17257 (comment)

Currently we have a more basic problem that I haven't learned how to handle rstMessage invocation in test, it just throws AssertionDeffect by some reason, which is ridiculous :-(

the problem is this code:

  MsgKind* = enum          ## the possible messages
    meCannotOpenFile = "cannot open '$1'",
    meExpected = "'$1' expected",
    ...
    meGeneralParseError = "general parse error",
    meInvalidDirective = "invalid directive: '$1'",
    mwRstStyle = "RST style: $1"

proc whichMsgClass*(k: MsgKind): MsgClass =
  ## returns which message class `k` belongs to.
  case ($k)[1]
  of 'e', 'E': result = mcError
  of 'w', 'W': result = mcWarning
  of 'h', 'H': result = mcHint
  else: assert false, "msgkind does not fit naming scheme"

relies on $k which doesn't reflect the ast symbol of the enum

Additional Information

1.5.1 07df4fe

proposed solution

I'd like all 3 things below:

fix 1

use range:

const
  MsgKindErrors = meCannotOpenFile..mwRedefinitionOfLabel
  MsgKindWarnings = mwRedefinitionOfLabel.next..mwRstStyle
  MsgKindHints = mwRstStyle.next..MsgKind.high

then:

proc whichMsgClass*(k: MsgKind): MsgClass =
  case k
  of MsgKindErrors: mcError
  of MsgKindWarnings: mcWarning
  of MsgKindHints: mcHint

fix 2

replace

    # TODO: find out hot to catch warning here instead of throwing a defect
    echo input8
    expect(AssertionDefect):
      let output8 = input8.toHtml

(eg from #17257)
accordingly now that the bug gets fixed, ditto for all (or most) of the other expect(AssertionDefect):

fix 3 + proposal: enumutils.symbolName

(EDIT: => #17281)
not needed to fix this issue but still useful in general (and would provide an alternative way to fix this): add proc nativeString(a: enum): string which returns the native string for an enum, ignoring the string definition. This should address remaining concerns for using this kind of enums:

type A = enum
  a0 = "human name"

(refs #15775 (comment))
since it gives both stringified form (most common case) as well as access to native name ("a0"):

let x = A.low
assert $x == "human name" # common case
assert nativeString(x) == "human name" # when needed
@a-mr
Copy link
Contributor

a-mr commented Mar 7, 2021

To have symbolName (fix 3) is generally useful. And it's closest to the original idea of code $k[1]; to change it to k.symbolName[1]

@timotheecour
Copy link
Member Author

@a-mr fix 3 has been merged so you can go ahead and use it to close this issue

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 a pull request may close this issue.

2 participants