Add pos argument to tryStep and return range of proof via proofContext#351
Add pos argument to tryStep and return range of proof via proofContext#351DikieDick wants to merge 1 commit into
Conversation
|
pimotte
left a comment
There was a problem hiding this comment.
I think the code looks fine, but I do have some questions:
- Just to check, this PR targets main intentionally?
- Was there a way I could easily test these API methods interactively?
- Does it make sense to unit test the commandExecutor?
- Do the proofContext changes have an implication for the Lean functionality when backmerging this into mapping?
Yes, as River currently works best (maybe even 'only') on main.
Apart from using River's The main problem this solves is that River would otherwise only be able to start a petanque context at the cursor position, instead of a blank input area.
Yes, but we can't fully test it without having a valid LSP in the background.
To my knowledge all this API stuff is only ever used for River, which relies on Rocq |
|
I think it does make sense to unit test the executor with a mocked client. While it's not much code, I think it's good to err on the side of testing a bit too much, and it also documents the assumptions we make on the LSP output for purposes of the executor. Especially if it's easier to setup, a test with the real client but mocked LSP messages would also be a good option. |


Proof.andQed., but works for scripts withoutProof.as well, see the regex)