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.

Sign up for GitHub

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

Jump to bottom

sync: consider using a different TOML parser #187

Open
ee7 opened this issue Feb 21, 2021 · 5 comments
Open

sync: consider using a different TOML parser #187

ee7 opened this issue Feb 21, 2021 · 5 comments
Labels
cmd: sync kind: refactor Change to production code that does not fix a bug or add a feature

Comments

@ee7
Copy link
Member

ee7 commented Feb 21, 2021 •
edited
Loading

We don't need to support the entire TOML syntax, just the syntax of:

  • # begins a comment
  • some-uuid = bool includes/excludes a UUID

Using our own parser would:

  • Allow us to remove our largest dependency
  • Improve performance
  • Reduce binary size
  • Possibly make it easier to support maintainer-added (non-generated) documentation comments, if that's something we want to do

To illustrate the final point, given a tests.toml file:

# first test description
bc310baa-ceae-4cb5-a656-20b7d2bbf1fe = true

# maintainer-added documentation comment

# second test description
3430d237-1ec8-4889-8f2d-17985d82e809 = false

And the Nim program:

import pkg/parsetoml

let toml = parseFile("tests.toml")

echo toml # see also `echo toml.repr`

The output is:

bc310baa-ceae-4cb5-a656-20b7d2bbf1fe = true
3430d237-1ec8-4889-8f2d-17985d82e809 = false

That is, as you might expect, the comments aren't available in the end data structure:

TomlValueRef = ref TomlValue

TomlValue = object
  case kind*: TomlValueKind
  of TomlValueKind.None:
    nil
  of TomlValueKind.Int:
    intVal*: int64
  of TomlValueKind.Float:
      floatVal*: float64
      forcedSign*: Sign

  of TomlValueKind.Bool:
    boolVal*: bool
  of TomlValueKind.Datetime:
    dateTimeVal*: TomlDateTime
  of TomlValueKind.Date:
    dateVal*: TomlDate
  of TomlValueKind.Time:
    timeVal*: TomlTime
  of TomlValueKind.String:
    stringVal*: string
  of TomlValueKind.Array:
    arrayVal*: seq[TomlValueRef]
  of TomlValueKind.Table:
    tableVal*: TomlTableRef

See also:
@ee7 ee7 mentioned this issue Feb 21, 2021
Closed
@ErikSchierboom
Copy link
Member

Allow us to remove our largest dependency

I don't really care much about this :) What would be major benefit be of removing this. Compile time? Size?

Improve performance

I think the performance benefit would be so minor that users won't actually notice

Reduce binary size

Which is nice, but the binary is already very small.

Possibly make it easier to support maintainer-added (non-generated) documentation comments, if that's something we want to do

While I could see the use for this, we should first decide how we want to allow maintainers to document decisions regarding tests.

@iHiD
Copy link
Member

iHiD commented Feb 24, 2021

I don't think it's worth the effort for this. Dependencies are good, not bad. We want to have to maintain less, not more stuff :)

So I think we should leave this, at least until we've got everything else done and we launch. Then we can always revist the conversation.

@ErikSchierboom
Copy link
Member

As we're now actively supporting arbitrary keys in our tests.toml files, we should keep on using the parsetoml library as otherwise we'd be effectively writing our own toml library. I think this issue can be closed. Do you agree @ee7?

@ee7 ee7 added cmd: sync kind: refactor Change to production code that does not fix a bug or add a feature labels May 10, 2021
@ee7
Copy link
Member Author

ee7 commented Jun 20, 2021

Slow reply, sorry.

I'll keep this open for now because we could also consider eventually moving to status-im/nim-toml-serialization. That'd make sense if we also eventually moved to status-im/nim-json-serialization.

@ee7 ee7 changed the title sync: consider using a simpler parser for tests.toml sync: consider using a different TOML parser Jun 20, 2021
@ee7
Copy link
Member Author

ee7 commented Aug 16, 2023

Just while I'm looking through the issues...

The situation here is still the same:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cmd: sync kind: refactor Change to production code that does not fix a bug or add a feature
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ErikSchierboom @iHiD @ee7