Decouple language and configuration from the Topiary library #643
Conversation
|
Note: The significant cuts to this repo, in 14e756f, seem to really be playing havoc with (Additionally, I believe there is something wrong with the Nix development shell. I'm getting toolchain mismatches all over the place and no amount of |
|
I have fixed the |
ErinvanderVeen
force-pushed
the
engine-separation
branch
from
844336d to
bd38c53
Compare
ErinvanderVeen
force-pushed
the
engine-separation
branch
5 times, most recently
from
5bc82d0 to
82d2c03
Compare
There was a problem hiding this comment.
Looks like I can review my own PR, but I cannot approve it 😅
Topiary has accumulated a lot of tech debt in order to push features. While this refactor is quite "ruthless" in what it cuts, I think it's a good first step to start paying off that debt 👍 I've only given the code a cursory look through, but it looks mostly fine.
General comments:
-
It's a shame we've had to (temporarily) remove the Playground and all the work that went into it. I understand and accept the justification, but I think it's worth doing the following in conjunction with this PR:
- Tag the previous commit on
mainwith something likeplaygroundorold-playground, so it can be found easily. - Create a new issue that calls for the playground to be reinstated, pointing to that tag (for reference) and documenting the reasons why it was removed by this PR, so a more long-term solution can be sought.
The Playground is a great advocate/demo for new-comers, so (imo) reinstating it should be a matter of utmost priority.
- Tag the previous commit on
-
I may have misunderstood, but I thought removing
tree-sitter-facadeas a dependency was one of the aims of this PR? It's still in there and being used. -
Not for this PR -- it's already gone on long enough -- but I think there's still plenty of scope for refactors and code improvements, to make Topiary more maintainable. (The work that went into this decoupling PR is a testament to how much debt had accumulated!) Perhaps we can look into this and plan some issues around it.
Also, as contributors, we should actively encourage the "Scouts Rule": Leave the code better than you found it. (Perhaps we can put it in
CONTRIBUTING.mdor a code of conduct doc...) It pays dividends, over time, despite the making PRs somewhat non-unitary.
| @@ -30,28 +23,20 @@ let | |||
| "topiary" | |||
| "topiary-queries" | |||
| "topiary-cli" | |||
| "topiary-playground" | |||
| "tests" | |||
| ]; | |||
| }; | |||
|
|
|||
| nativeBuildInputs = with pkgs; | |||
| [ | |||
| binaryen | |||
There was a problem hiding this comment.
I think binaryen is a WASM-thing, too.
topiary-cli/tests/sample-tester.rs
Outdated
There was a problem hiding this comment.
I wonder if the sample tester (and all the input/expected pairs) should be part of topiary-queries, rather than topiary-cli?...
This includes moving the sample tests to Topiary cli (because topiary no longer knows about languages). Additionally, bash was reverted back to an older version due to a regression that needs to be investigated.
At this point, the LanguageDefinition contained only a single value, and was thus mostly useless
ErinvanderVeen
force-pushed
the
engine-separation
branch
from
82d2c03 to
eb002d7
Compare
| @@ -23,7 +23,6 @@ prettydiff = { workspace = true } | |||
| regex = { workspace = true } | |||
| serde = { workspace = true, features = ["derive"] } | |||
| serde_json = { workspace = true } | |||
| toml = { workspace = true } | |||
| tree-sitter-facade = { workspace = true } | |||
| unescape = { workspace = true } | |||
There was a problem hiding this comment.
I see that the tree-sitter-language dependencies are still required in the core engine. Maybe not the goal of this PR, but it would be great to be able to abstract the language as well so that it can be passed as a function argument without needing to build the languages we do not need.
There was a problem hiding this comment.
That is absolutely the plan! Thanks for your feedback!
|
Obsoleted by #672 |
topiary/src/configuration.rs: Definitions and implementations of configuration serialisation (TOML) format, plus default configuration (read at build time).Error handling: Move serde deserialisation error handler to CLI(CLI already has this.)topiary/src/language.rs: Definitions and implementations ofLanguage, with mappings to Tree-sitter grammars, andSupportedLanguagesubset.Languagetype for use in library.SupportedLanguageto CLI. (Note: At which point the distinction betweenLanguageandSupportedLanguagecan be collapsed.)Languagetype.topiary/src/lib.rs: Main entrypoint for the public API of the library. Ensure it matches with the changes described above; tying-up remaining loose ends.topiary/src/tree_sitter.rs: Tree-sitter interface for library. Ensure it matches with the changes described above; tying-up remaining loose ends.Resolves #606