Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Virtualtext formatting and alignment fixes#4473

Merged
hsanson merged 1 commit intodense-analysis:masterdense-analysis/ale:masterfrom
systemmonkey42:virtualtextfixsystemmonkey42/ale:virtualtextfixCopy head branch name to clipboard
Mar 31, 2023
Merged

Virtualtext formatting and alignment fixes#4473
hsanson merged 1 commit intodense-analysis:masterdense-analysis/ale:masterfrom
systemmonkey42:virtualtextfixsystemmonkey42/ale:virtualtextfixCopy head branch name to clipboard

Conversation

@systemmonkey42
Copy link
Copy Markdown
Contributor

@systemmonkey42 systemmonkey42 commented Mar 8, 2023

Example implementation of the enhancement described in #4472

This include minimum and maximum columns expected for messages to start, and filtering to ensure only
one error appears per line of code.

This change converts this:

2023-03-09_09-30-20

To this:

2023-03-09_09-33-45

Copy link
Copy Markdown
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

Could you do the following?

  1. Add videos or GIFs into the PR description to show a "before" and "after" for the changes.
  2. Localise the new blocks of code into a new function or two to stop functions getting any larger.
  3. Add comments in code to explain what the new code is doing.
  4. Add Vader tests for code where you can. See: test/test_virtualtext.vader.
  5. Change text alignment to only be applied when a new settings are enabled. I may want this on or off by default.
  6. Add documentation for the new settings and how they work.
  7. Set default values for new settings.

Aligning text is a good idea. In my editor, I want all warnings and errors to appear at the end of lines, but not so far to the right that they don't fit on the screen. If you see a file full of errors... fix the file so that doesn't happen, or turn the linter off.

@systemmonkey42
Copy link
Copy Markdown
Contributor Author

Screenshots added. I hope they are ok.

If you see a file full of errors... fix the file so that doesn't happen, or turn the linter off.

As you can see, a few characters in javascript, and you have pages of errors, going backwards and forwards in the file.
The first line containing an errors isn't even helpful in finding the error.

@w0rp
Copy link
Copy Markdown
Member

w0rp commented Mar 9, 2023

The GIFs look good. 👍

The other issue I linked in the issue covers cleaning up spam from linters.

@systemmonkey42
Copy link
Copy Markdown
Contributor Author

systemmonkey42 commented Mar 11, 2023

  • Add videos or GIFs into the PR description to show a "before" and "after" for the changes.
  • Localise the new blocks of code into a new function or two to stop functions getting any larger.
  • Change text alignment to only be applied when a new settings are enabled. I may want this on or off by default.
  • Add comments in code to explain what the new code is doing.
  • Add documentation for the new settings and how they work.
  • Set default values for new settings.
  • Add Vader tests for code where you can. See: test/test_virtualtext.vader.

Added support for buffer local settings, and setting column as a percentage of window width.

I can run the existing tests ok, but I'm not sure what tests I can add.

@w0rp w0rp self-assigned this Mar 15, 2023
@w0rp w0rp added this to the Version 4.0.0 milestone Mar 15, 2023
@pusewicz
Copy link
Copy Markdown

Can't wait for this!

Copy link
Copy Markdown
Contributor

@hsanson hsanson left a comment

Choose a reason for hiding this comment

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

Good improvement to visuals and I have confirmed it does not break things at least on my side. We can think about tests latter.

@hsanson hsanson merged commit 41e12fd into dense-analysis:master Mar 31, 2023
mnikulin pushed a commit to mnikulin/ale that referenced this pull request Nov 12, 2023
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.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.