Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Bunch of changes/fixes after finally upgrading #40

Merged
merged 6 commits into from
Feb 6, 2025

Conversation

svermeulen
Copy link
Collaborator

I upgraded to latest and had to make the following changes to get it compatible with my workflow. Apologies for shoving this all in one PR.

  • Updated teal type definition file based on current state of teal master
  • Fixed to report all diagnostics correctly. For whatever reason this was fixed by passing in filename to tl.parse_program
  • Refactors logging to be simpler. No longer supports named fields. The string placeholders should always be {} or {formatting} where formatting is a string.format compatible formatting string or a few special cases like @ for serialize
  • Added a bunch more logging as necessary while I was debugging. Should all be disabled unless --verbose is provided
  • Fixed tree sitter issue. I was finding that 'hover' was incorrectly providing data for the previous column, so I changed "Document:_tree_sitter_token" to interpret the 'end_point' value tree sitter provides as exclusive. I was unable to confirm that this is expected from tree sitter docs but this fixes my issue
  • Fixed bug with completion request, where it would fail if called from the end of the line in neovim. I assume this was because neovim sends the cursor position, which is technically outside the range of the current line, so I subtraced character by one to account for this in MiscHandlers:_on_completion.
  • Fixed bug where some methods wouldn't be returned by completion when arg type == tl.typcodes.RECORD

…ster

* Fixed to report all diagnostics correctly.  For whatever reason this was fixed by passing in filename to tl.parse_program
* Refactors logging to be simpler.  No longer supports named fields.  The string placeholders should always be {} or {formatting} where formatting is a string.format compatible formatting string or a few special cases like @ for serialize
* Added a bunch more logging as necessary while I was debugging.  Should all be disabled unless --verbose is provided
* Fixed tree sitter issue.  I was finding that 'hover' was incorrectly providing data for the previous column, so I changed "Document:_tree_sitter_token" to interpret the 'end_point' value tree sitter provides as exclusive.  I was unable to confirm that this is expected from tree sitter docs but this fixes my issue
* Fixed bug with completion request, where it would fail if called from the end of the line in neovim.  I assume this was because neovim sends the cursor position, which is technically outside the range of the current line, so I subtraced character by one to account for this in MiscHandlers:_on_completion.
* Fixed bug where some methods wouldn't be returned by completion when arg type == tl.typcodes.RECORD
@svermeulen svermeulen changed the title Bunch of changes/fixes after finally upgrading WIP: Bunch of changes/fixes after finally upgrading Jan 27, 2025
@svermeulen
Copy link
Collaborator Author

In terms of the test failures - These are all due to the change to make the end_point exclusive. If that fix is correct, then I can fix the tests too by subtracting one from the given end point index for these failures

@svermeulen
Copy link
Collaborator Author

After more usage of this branch I found this teal issue

Which makes me wonder if we should should uncomment the 'quick' approach in Document:type_information_for_tokens, though I find that causes other problems too

@FourierTransformer
Copy link
Collaborator

Thanks for giving the new code a go! I'm liking the logging changes. Everything seems good overall. I'll try and test in Sublime in a few days to see if the subtracted character makes a difference. I'm not sure how other languages' LSPs handle passing the cursor "position". I guess at some point we'll need to figure out which editors to "support" (apart from neovim, likely vscode but there is https://github.com/teal-language/vscode-teal, but i'm not sure how it uses the language server).

Which makes me wonder if we should should uncomment the 'quick' approach in Document:type_information_for_tokens, though I find that causes other problems too

Yeah, I ended up using both because they worked in different situations well. I'd opt to uncomment the "quick" route for now and make a tracking issue (with some examples use cases where things fail) to followup from the resolution from teal-language/tl#921.

@svermeulen
Copy link
Collaborator Author

I'm not sure how other languages' LSPs handle passing the cursor "position"

Yeah, same, I've only tested neovim

I'd opt to uncomment the "quick" route for now and make a tracking issue (with some examples use cases where things fail) to followup from the resolution from teal-language/tl#921.

I'm not sure if it's better on or off at the moment, since I think there's issues in both cases. I noticed that you had it off so I'm guessing you found it didn't help enough? I'll try both for awhile to see which one seems better in my workflow

@FourierTransformer
Copy link
Collaborator

Finally had some time to test this out. The -1 changes didn't seem to affect Sublime Text (and hopefully others)! However your branch did break the "function signature" completion. Not sure if it shows in neovim without a plugin.

Yeah, looking back I remembered that the "quick" would get weird errors where it wouldn't find the actual token information, even with +1/-1 line/token stuff and the symbol based lookup was more reliable. It mostly ended with a bad ux where completion would sometimes work, and I wanted to keep the ux consistent, so I opted not to use it. We could probably switch to it if the issue in tl is addressed.

@FourierTransformer
Copy link
Collaborator

Also I think after we get this merged, I'll finish up the last of #26, and play around with neovim installs and see if we can streamline installs for people. I should have some time coming up here to actually work on this.

@svermeulen
Copy link
Collaborator Author

Up until now I haven't used "signature completion" and didn't know it existed. But having tried that plugin now I see how useful that is!

I was able to repro the breakage there and fixed it in the most recent commit by subtracting one character from provided position, similar to what we're doing in _on_completion.

I haven't read the LSP spec in detail or tested in other environments to be really sure this correct. But for my purposes, within neovim, this works well.

BTW also - I tested some more and also find that things work better overall by leaving the 'quick' approach commented out. That fixes some bugs but causes more, for me at least.

Copy link
Collaborator

@FourierTransformer FourierTransformer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alrighty, it's all good on my end. I think we are good to send it. LGTM!

@svermeulen svermeulen changed the title WIP: Bunch of changes/fixes after finally upgrading Bunch of changes/fixes after finally upgrading Feb 6, 2025
@svermeulen svermeulen merged commit d823299 into teal-language:main Feb 6, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants