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

Refactoring #1

Open
alxwrd opened this issue Apr 30, 2018 · 3 comments
Open

Refactoring #1

alxwrd opened this issue Apr 30, 2018 · 3 comments

Comments

@alxwrd
Copy link
Owner

alxwrd commented Apr 30, 2018

_format_fields seems clunky. The Javascript implementation in ratlog/ratlog.js seems a lot clearer

  const fieldString = Object.entries(fields || {})
    .map(([k, v]) => {
      const key = formatField(k)
      const value = formatField(v)
      return ` | ${key}${value && ': ' + value}`
    }).join('')

I also don't like the hacky newline escaping in escape. However,

newline = "\n"
print(repr("\\" + newline))
# '\\\n'

It's possible to string.replace("\n", "\\n"), but this just adds a special case for newlines. I don't know if this is better?

@jmsv
Copy link

jmsv commented Jul 20, 2018

ratlog.js defines a formatFields method that's called for key and value. Could we do something similar? e.g. (untested)

# Define this somewhere globally or within Log class
_format_field = lambda val: escape("|:", val) if val else ""

def create_field(entry):
    # Replace key & value string construction with _format_field call
    key = _format_field(entry[0])
    value = _format_field(entry[1])
    # Move colon from value definition to format string
    return " | {}: {}".format(key, value)

This might look tidier and aligns more to the reference implementation

@jmsv
Copy link

jmsv commented Jul 20, 2018

Regarding escaping: string.replace("\n", "\\n") seems like a bad idea imo. Fits the newline case but doesn't work with \t (tab) etc

@alxwrd
Copy link
Owner Author

alxwrd commented Jul 20, 2018

Regarding escaping: string.replace("\n", "\n") seems like a bad idea imo. Fits the newline case but doesn't work with \t (tab) etc.

Agree. I just can't find a way to say:

>>> def escape(chars, string):
...     for char in chars:
...         string = string.replace(char, "\\"+char)
...     return string
...

>>> escape("\n", "this\ncontains\nnewlines")
'this\\\ncontains\\\nnewlines'
>>> print(_)
this\
contains\
newlines

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

No branches or pull requests

2 participants