badicsalex / peginator

PEG parser generator for creating ASTs in Rust
MIT License
34 stars 3 forks source link

Reduce Dependencies #13

Closed npmccallum closed 1 year ago

npmccallum commented 1 year ago

The goal of this series of patches is to reduce dependencies throughout the peginator system. The first few patches remove dependencies that don't provide a lot of value. Afterwards, we restructure the peginator code to have the following structure:

      ┌───────────────────┐   ┌───────────────────┐
      │                   │   │                   │
      │ peginator_codegen │◄──┤ peginator_runtime ├──┐
      │                   │   │                   │  │
      └─────────┬────┬────┘   └───────────────────┘  │
                │    │                               │
                │    └──────────────────┐            │
                ▼                       ▼            │
      ┌───────────────────┐   ┌───────────────────┐  │
      │                   │   │                   │  │
      │  peginator (cli)  │   │    buildscript    │  │
      │                   │   │                   │  │
      └──────────────┰────┘   └────┰──────────────┘  │
                     ┇             ┇                 │
                     ┇  generates  ┇                 │
                     ▼             ▼                 │
                  ┌───────────────────┐              │
                  │                   │              │
                  │  generated code   │◄─────────────┘
                  │                   │
                  └───────────────────┘

There are a few places that still look like dependencies could be removed. The most obvious is colored which could be optional.

badicsalex commented 1 year ago

Damn, thanks for this, I've been meaning to do this split for ages. I'll review it right away.

badicsalex commented 1 year ago

All the code and changes look good.

I'm wondering though, how will this affect the discoverability and navigation on docs.rs and crates.io? Shouldn't all crates have a proper README, and some basic information (or links to the other crates) in the lib.rs files?

I'm thinking that something along these lines:

badicsalex commented 1 year ago

Both colored and nohash-hasher could be made optional. Colored is only needed for pretty tracing and error reporting, which is not really important for a lot of runtime-only users. In fact, the direct colored dependency could easily be removed from the cli crate by moving the reg error string formatting into runtime.

Nohash-hasher is only needed to speed memoization up somewhat. The speedup is not that big, and if you don't use memoization, it's useless. I think the generated code does not depend on it directly. It's a tiny little crate though, so it can also stay.

npmccallum commented 1 year ago

@badicsalex I have addressed your additional requests.

badicsalex commented 1 year ago

This looks great, thanks.

badicsalex commented 1 year ago

I'll make an effort to push this up to crates.io in the next few days.

badicsalex commented 1 year ago

I published the changes. It's important to note that my idea for the documentation was wrong, because it's somewhat pointless to document in a main.rs, because docs.rs will not process binary crates.

In the end I renamed peginator_runtime to peginator, and moved the CLI to peginator_cli.

Also, colored was made optional as a bonus.