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

Fixed problems with formatting the code with errors #84 #99

Merged
merged 1 commit into from
Mar 20, 2020

Conversation

speles
Copy link
Contributor

@speles speles commented Oct 4, 2019

Only changing of the regular expression is enough to fix the bug #84.

Though, separation of the data from stdout and stderr helps to avoid potential issues with "malicious" strings in the source (for example, inclusion of the string "Syntax error in test.cr:5:1: ooops" in the source will create phantom error on line 5 after the formatting).

@ndbroadbent
Copy link

Hello, it seems no-one has looked at this PR for a while, so I will try to install your fork of the vs code plugin. Will update my comment with instructions if I can get it working.

@ndbroadbent
Copy link

Oh also I was looking at your change (and the existing code), and I feel like it might be a bad idea to rely on stdout/stderr and a regex. It would be much nicer if crystal tool format had a known list of exit statuses for some different kinds of errors, so that vscode-crystal-lang knows exactly why something failed and respond appropriately. e.g. underline and show error for syntax errors, but show a more prominent error notification for other errors (e.g. require a missing file, or it crashes with an unknown error)

@paulcsmith
Copy link

Hi @RX14 @bew @bmulvihill! Could someone on the team merge this in? This is a pretty huge issue with the plugin since it completely wipes out the code if there is a syntax error. It may not be perfect since it uses a regex, but it seems to be much better than it is now

I would also be happy to be added to this repo to help get this through.

Thanks you so much!

@bew
Copy link

bew commented Mar 18, 2020

Hey @paulcsmith, I wasn't aware of this issue, can you confirm that the bug is gone with this PR ?

I don't use vscode myself, so if you say it's ok, I can merge it, but I'm not sure how to release a new version to vscode's marketplace, so i'm not sure a merge is enough in this case..

@paulcsmith
Copy link

@bew thanks for the lightning fast response. I've tested it and it does indeed fix the issue :D

For anyone curious, I used the instructions here: https://stackoverflow.com/a/54409592

To publish it you'll need the vsce npm package: https://code.visualstudio.com/api/working-with-extensions/publishing-extension

@faustinoaq may need to give you access, or maybe this just need to have a version bump to 0.4.x and be under a new org/team/whatever vscode gallery calls them

@paulcsmith
Copy link

Also syntax errors still show up :D

Screen Shot 2020-03-18 at 12 19 43 PM

@RX14 RX14 merged commit 238a3a1 into crystal-lang-tools:master Mar 20, 2020
@paulcsmith
Copy link

Thank you!

@bew
Copy link

bew commented Mar 20, 2020

Ah I was about to do it^^
Thanks @RX14

Anyone knows how to contact @faustinoaq ? So he can make a proper release for vscode' marketplace

@paulcsmith
Copy link

@speles Thanks so much for fixing this! Feel free to claim the $50 bounty I posted! https://www.bountysource.com/issues/69775273-whole-buffer-replaced-with-syntax-error

Once you do that I can confirm and you'll get your cash money!

@faustinoaq
Copy link
Member

New v0.4.0 published on Github also in Visual Studio Marketplace

Thank you so much @speles! 🎉 😄

@paulcsmith
Copy link

@speles Just another friendly reminder to claim the bounty https://www.bountysource.com/issues/69775273-whole-buffer-replaced-with-syntax-error :D

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.

6 participants