Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor Language interface #318

Merged
merged 4 commits into from
Feb 27, 2023
Merged

Conversation

Xophmeister
Copy link
Member

@Xophmeister Xophmeister commented Feb 23, 2023

The original purpose of this PR was to expose extension points to Tree-sitter's parser, at the library level, such that the binary could emit a serialised form of the parsed tree (i.e., to satisfy #11). After much thought, however, it is my opinion that it is not the library's job to provide this and is only offered by the binary as a "nice-to-have". Specifically:

  • Walking the syntax tree in Tree-sitter is relatively straightforward. Serialising the output is a harder task, but has nothing to do with formatting.
  • Exposing Tree-sitter is no different from using Tree-sitter directly.
  • As such, if the exposure was to provide an interface -- presuming, for the sake of argument, it's the library's responsibility -- then we'd have the job of reimplementing or wrapping Tree-sitter's functionality. Due to orphan rules, these may then need to be wrapped again in the binary.
  • That's a lot of wasted effort, when we could just have the binary also depend on Tree-sitter. (Currently, this requires zero effort, as the library and binary are still conjoined; see Migrate to a Cargo workspace, to keep library and binary distinct #295.)
  • What is needed, however, is Topiary's library's machinery to deal with various languages (including Dynamic loading of language support #4, once implemented), such that it can be passed to Tree-sitter without additional fuss.

This PR is therefore just a refactoring of the Language type: Rather than explicit methods, we use conversion traits instead, plus there's more documentation. It's not strictly necessary, but ought to make the subsequent code for the binary slightly more idiomatic.

@Xophmeister Xophmeister force-pushed the chris/syntax-output/expose-ts branch from 3258351 to 6884e50 Compare February 23, 2023 17:52
@Xophmeister Xophmeister changed the title Expose Tree-sitter Refactor Language interface Feb 23, 2023
@Xophmeister Xophmeister force-pushed the chris/syntax-output/expose-ts branch from 091b7ff to 47f53af Compare February 24, 2023 10:57
@Xophmeister Xophmeister force-pushed the nb/format_tree_sitter_query branch from ef2a789 to 3bb18b1 Compare February 27, 2023 10:22
Base automatically changed from nb/format_tree_sitter_query to main February 27, 2023 10:35
@Xophmeister Xophmeister force-pushed the chris/syntax-output/expose-ts branch from b8dace5 to 758d26d Compare February 27, 2023 11:45
@Xophmeister Xophmeister reopened this Feb 27, 2023
@Xophmeister Xophmeister marked this pull request as ready for review February 27, 2023 11:55
@Xophmeister
Copy link
Member Author

(Rebasing was not very clean; now fixed.)

@torhovland
Copy link
Member

If you change this:

-impl TryFrom<Language> for PathBuf {
+impl TryFrom<&Language> for PathBuf {
     type Error = FormatterError;
 
-    fn try_from(language: Language) -> FormatterResult<Self> {
+    fn try_from(language: &Language) -> FormatterResult<Self> {
         let basename = Self::from(match language {
             Language::Bash => "bash",
             Language::Json => "json",
@@ -177,8 +177,8 @@ impl TryFrom<PathBuf> for Language {
 ///
 /// Note that, currently, all grammars are statically linked. This will change once dynamic linking
 /// is implemented (see Issue #4).
-impl From<Language> for Vec<tree_sitter::Language> {
-    fn from(language: Language) -> Self {
+impl From<&Language> for Vec<tree_sitter::Language> {
+    fn from(language: &Language) -> Self {

then you can also do this:

-        (*self).try_into()
+        self.try_into()
     }
 
     /// Convenience alias to return the Tree-sitter grammars for the Language.
     pub fn grammars(&self) -> Vec<tree_sitter::Language> {
-        (*self).into()
+        self.into()

@Xophmeister
Copy link
Member Author

@torhovland My -- probably naive -- reasoning for this is that the TryFrom implementations "feel more useful" (don't ask me to quantify or justify this!) against the concrete type, rather than its reference, for the sake of an API. However, I'm not very certain about that, given the consequences of ownership.

Do you think, for the API, implementations against the references would be more idiomatic?

@torhovland
Copy link
Member

Well, the From and TryFrom implementations are typically added to serve a need. So when you do self.into() and the compiler tells you that

the trait `From<&language::Language>` is not implemented for `Vec<tree_sitter::Language>`

I would take that as a hint that I need that implementation.

@Xophmeister
Copy link
Member Author

Good point. I suppose what I'm trying to do is anticipate the need of future downstream API users, but perhaps that is futile, or at the very least, secondary compared to current needs.

@Xophmeister Xophmeister merged commit dfe542f into main Feb 27, 2023
@Xophmeister Xophmeister deleted the chris/syntax-output/expose-ts branch February 27, 2023 14:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants