Skip to content

Conversation

RedBeard0531
Copy link
Contributor

This is a POC. If the general concept is approved, I'll test, document, etc.

This is an alternative to #7070

else:
format($arg, option, res)

proc impl(pattern: NimNode, shouldUnescape: bool): NimNode =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to move this above the runableExamples so the diff is huge. The only actual change was adding the makeStrLit template and calling on lines 252 and 285

while i < f.len and f[i] != '}' and f[i] != ':':
subexpr.add f[i]
inc i
if i == f.len:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check was added in commit 2 to improve the error message when " is used incorrectly

proc unescape*(s: string, prefix = "\"", suffix = "\""): string {.noSideEffect,
rtl, extern: "nsuUnescape".} =
## Unescapes a string `s`.
## Unescapes a string `s` using nim's string escape rules.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if that is its purpose. I imagined it to use some lowest common denominator escape rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not too opinionated on where this functionality belongs, but it does seem useful in stdlib. Does macros.nim sound better since it is most useful when writing macros?

@dom96
Copy link
Contributor

dom96 commented Jan 12, 2018

So it's either:

  • % and fmt

or

  • just fmt but with modified semantics to get rid of the raw string literal semantics?

@GULPF
Copy link
Member

GULPF commented Jan 12, 2018

@dom96 This PR stills keeps %. It fixes the case where the text outside of {} needs to be treated as a normal string, while the text inside {} needs to be treated as a raw string (note that this is not the same thing as getting rid of the raw string literal semantics).

This is not possible to solve with %, since both %"" and %r"" would give the wrong result. To make "{foo}\n{\bar}" work with % you would need to use %"{foo}\n{\\bar}" instead, which is a bit silly and might be impossible to syntax highlight in a sane way.

@dom96
Copy link
Contributor

dom96 commented Jan 12, 2018

To make "{foo}\n{\bar}" work with % you would need to use %"{foo}\n{\\bar}" instead, which is a bit silly and might be impossible to syntax highlight in a sane way.

In what way is that silly? That's what I would expect. If you want a literal \ in your string you need to escape it.

@RedBeard0531
Copy link
Contributor Author

I think it is a distinction in whether you view it as "writing a string that is passed to the fmt macro" or "writing an fmt string". I think your description makes sense if you think of it like the former, which is technically correct but less useful. I think we should make fmt work like the later, similar to how when using re"" you think about writing a regex rather than writing a string that gets passed to re. Isn't that the goal when writing a DSL?

@GULPF
Copy link
Member

GULPF commented Jan 12, 2018

It becomes a big problem with syntax highlighting. A good highlighter should highlight text inside {} as code, but that means that there will be no visual indication that \ is actually the start of an escape sequence. Besides, fmt"{foo}\n{\bar}" uses the raw string syntax so there is no reason to assume that the string follows normal escape rules (I would assume the opposite even - that it follows the escape rules of a raw string).

@Araq
Copy link
Member

Araq commented Mar 2, 2018

Thank you for this PR but we decided for & or .fmt which doesn't require this additional complexity.

@Araq Araq closed this Mar 2, 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.

4 participants