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

Add status bar instead of error notifications (VSC extension) #826

Merged
merged 4 commits into from
Dec 30, 2023

Conversation

DervexDev
Copy link
Contributor

This change allows users to hide formatter errors in the extension settings. These error notifications are especially annoying when using Format On Save or Format On Paste options so it makes sense to disable them optionally.

image

@DervexDev DervexDev changed the title Add option to hide formatting errors Add option to hide formatting errors (VSC extension) Nov 19, 2023
Copy link

codecov bot commented Nov 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5de9a1d) 97.02% compared to head (4fad65b) 97.03%.
Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #826   +/-   ##
=======================================
  Coverage   97.02%   97.03%           
=======================================
  Files          16       16           
  Lines        5989     5995    +6     
=======================================
+ Hits         5811     5817    +6     
  Misses        178      178           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JohnnyMorganz
Copy link
Owner

Yeah, I don't particularly like these errors too much. I was wondering whether we should just remove them completely and either replace them with:

a) proper VSCode diagnostics. But is it even worth it? Seems more like a role for a language server / linter than the formatter

b) an icon on the status bar - similar to how Prettier does it

@DervexDev
Copy link
Contributor Author

I think the second option is better. I'm going to implement this in like half an hour.

@DervexDev
Copy link
Contributor Author

Okay so I added status bar item and removed unnecessary setting, take a look:

Success:
image

Failure:
image

@DervexDev DervexDev changed the title Add option to hide formatting errors (VSC extension) Add status bar instead of error notifications (VSC extension) Nov 19, 2023
@JohnnyMorganz
Copy link
Owner

Looks pretty great, thanks for taking the time!

I'll try it myself next week and hopefully get this merged in.

(And wow, that error message is so verbose..., I never really realised)

@LastTalon
Copy link
Contributor

a) proper VSCode diagnostics. But is it even worth it? Seems more like a role for a language server / linter than the formatter

b) an icon on the status bar - similar to how Prettier does it

People typically use prettier with eslint in my experience, so they get diagnostics through that. It would be best if selene could handle a stylua plugin.

@JohnnyMorganz
Copy link
Owner

Thank you very much for contributing it. So sorry for the delay in testing it out, took a bit more than a week 😅

One minor thing I noticed was when you switch files on StyLua error, it still shows up as an error - it should probably clear itself when the active text editor changes. Maybe the item should even hide on non-Lua files.

I really like it displaying in the Status Bar. I did find something new in VSCode though, a "Language Status Item": https://code.visualstudio.com/api/references/vscode-api#LanguageStatusItem, which seems like another great spot to put this, as I don't think we need to take up extra space. For example, for eslint:
image

And this also gives the user the option to pin it to the bar if they want.

I took the liberty to build off your work and do the above changes, hope you don't mind. Thank you once again!

@JohnnyMorganz JohnnyMorganz merged commit c3a9d97 into JohnnyMorganz:main Dec 30, 2023
19 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.

3 participants