configlet icon indicating copy to clipboard operation
configlet copied to clipboard

sync: consider using a different TOML parser

Open ee7 opened this issue 4 years ago • 5 comments

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:

  • https://nimparsers.github.io/parsetoml/index.html
  • https://github.com/NimParsers/parsetoml

ee7 avatar Feb 21 '21 12:02 ee7

Hi!

I've been taking a look to set up the environment locally and this is something I'd like to tackle if no one else is on it yet.

Without having the full context of the project (yet) my first thought is creating an .env file with the port configuration there. WEBSITE_PORT=3020

  • docker-compose 1.5+ automatically reads .env files, so we could change the port for ${WEBSITE_PORT} on docker-compose.full.yml
  • bin/start would need to read the env file, which should be as easy as set -o allexport; source .env; set +o allexport and then using the env variable to load the port

As far as I've seen those are the 2 places where the port is used / hardcoded. Any other places? anything else I might have missed?

Thanks for everything!

TJSoler avatar Apr 22 '21 07:04 TJSoler

@TJSoler. Thanks for taking this on. @ErikSchierboom is away this week but will get back to you next week :)

iHiD avatar Apr 22 '21 12:04 iHiD

Cool, I'm in no rush. I will probably do it anyway because it's a really straightforward thing and I want to tinker with the new site and need to change ports. So what I might do is create my original idea as a pull request and continue the conversation there.

I'm more than open to change strategies once @ErikSchierboom has the time to check it out, or if the PR ends up closed, that's ok too.

TJSoler avatar Apr 22 '21 17:04 TJSoler

Sooo... Maybe I jumped the gun. I still believe it's easy but as far as I can see it would need to be coordinated between exercism/config, exercism/website and this repo.

I am still willing to work on this, but I don't have the context for the whole organisation wide repos / source / architecture. So I will wait for you to have time and have some input before progressing.

This is my work so far: https://github.com/exercism/development-environment/compare/main...TJSoler:define-website-port-in-env-file

It works, but internal links on the website are still hardcoded to the port.

TJSoler avatar Apr 22 '21 19:04 TJSoler

@TJSoler This would indeed also require changes to the website and config repo. We're currently very busy with preparing for the beta, so I think I won't get around to doing this for a while. I'm fine with keeping this open though, as I do think it is a good suggestion.

ErikSchierboom avatar Apr 28 '21 09:04 ErikSchierboom

I agree this is (and should be) low priority. What I think I might do if you agree, I might start working on the changes on website and config repos, maybe even open the PR to track the changes and open discussion, but mark the PR as [WIP], wait for the dust of V3 to settle. I wouldn't want to add anything non-important to your plate.

It's also a really good opportunity for me to see how the repos connect with eachother and to understand the project a bit.

Congrats on an amazing work BTW, really cool what I've seen so far.

TJSoler avatar Apr 28 '21 09:04 TJSoler

@TJSoler Yeah that's totally fine. We probably won't be able to review for a while, but be our guest!

ErikSchierboom avatar Apr 28 '21 11:04 ErikSchierboom