go-org icon indicating copy to clipboard operation
go-org copied to clipboard

Using `go-org` to build a multi-purpose LSP

Open M3kH opened this issue 3 years ago • 4 comments

Hi I would like to experiment with go-org to build a LSP for org. I'm new in go and so my observations might be not correct.

My intention is locate all the #+begin_src and use the language tag and grab the content and proxy to an appropriate LSP, which I control for proper integration.

I manage to do this with orgajs because the AST contains the position (start, end) of the Node.

To achieve the same thing with go-org I identify some approaches, but since I'm not an expert I'm looking for suggestions:

  1. Convert Document.tokens into public Document.Tokens, this is not perfect because for parsing information like language I need to go to AST format, but seems the easiest.
  2. Get the position from the block with either Document.parseOne or Document.parseMany by making sure that all Node are converted into *Node so that I can use the Pointer reference in a Linked List as [*Node]NodePosition.
  3. Move from type Node interface into type Node struct to include more information.

Although I'm willing to do a PR if this is from common interest, I'm asking for help to learn and to understand what is the best "golang way" for such case.

Thank you in advance.

M3kH avatar Nov 02 '22 23:11 M3kH

Hey,

Option 2 sounds best to me. We could also embed a position{Start, End int} struct into each node instead of keeping the positions in a separate map (then again, just different tradeoffs; the map would make it easier to only keep positions for blocks and not inline nodes... just brainstorming here)

I don't think I should say anything about what's the true golang way; I've committed quite some atrocities against it in this repo :D, e.g. https://github.com/niklasfasching/go-org/commit/14900e9/

So whatever you pick, happy for a PR. I'll eventually look into implementing this myself otherwise, no idea when though.

niklasfasching avatar Nov 09 '22 14:11 niklasfasching

Option 2 sounds best to me. We could also embed a position{Start, End int} struct into each node instead of keeping the positions in a separate map (then again, just different tradeoffs; the map would make it easier to only keep positions for blocks and not inline nodes... just brainstorming here)

So I went trough and try some, and consider one more option, here my learnings:

  • Option 2) Is doable, and indeed you get to selectively apply the position. Although convert from Value to Pointer seems affecting Performance and delivers a "less nice" API, again I'm not an expert. But at the end implementation didn't felt right.

  • Option 3) I try to adding fields to Node but I didn't foresee the complexity of inline nodes. So I drop the solution half way. I've embrace the simplicity that some Node are just string and came up with Option 4.

  • Option 4) I wrap Node into struct RangedNode which I add the Position next to it. I got this somehow working, but I got some question in prospect of a LSP.

LSP Considerations:

  • Org-mode is parsed mainly by line, and rightfully so go-org Scanner is setup with the default Read in line break. Although, LSP works a lot of time with Range query, so is useful to store the position also in character in the document. Can be valuable to go-org add Range as same LSP defines it?

    struct Range {
      Start { Line int, Char int }
      End { Line int, Char int }
    }
    
  • Would you rather than wrapping consider moving away from interface Node which leaves the ambiguity (and the simplicity) to node just be a string, to just be a more "complex" type? like:

    struct Node {
       Content string,
       Range Range,
    }
    

    Is this a Breaking change?

M3kH avatar Nov 11 '22 22:11 M3kH

Hey! Thx for the PR. I'll try to take a look this weekend :).

niklasfasching avatar Nov 16 '22 17:11 niklasfasching

Sorry for not getting back on this as planned. Looks good!

Are you blocked by this getting merged?

I'm still pondering how I feel about wrapping in RangedNode (or the suggested Node struct rather than interface). No strong feelings against it, just wondering whether it could be even nicer to instead cast to RangedNode to keep the interface unchanged, e.g.

type RangedNode interface {
        Node
	GetRange() Range
}

type Range struct{ Start, End Position }

func (r Range) GetRange() Range { return r }

type Comment struct{ 
    Range
    Content string 
}

func (d *Document) parseComment(i int, stop stopFn) (int, Node) {
	return 1, Comment{Range{...}, d.tokens[i].content}
}

_, node := parseComment(...)
node.(RangedNode).GetRange()

Maybe I'm just being nitpicky here. Any thoughts?

niklasfasching avatar Nov 26 '22 13:11 niklasfasching