Skip to content

Conversation

@everett1992
Copy link

Some signs would not be removed from the sign column when their
diagnostic was fixed.

Debugging revealed that signs_to_add would include two signs for the
same line. state.signsis keyed by line number so the second sign for
a line will overwrite the first when signs_to_add is recorded in state.
The first sign's id is lost when it's overwritten so it can't be
removed.

signs_to_add included two signs for the same line because itertools
group_by only groups consecutive runs (it's more efficient, you only
need to remember the current group and can flush it when the key
changes).

This quick fix sorts diagnostics by line number so that group_by gives
us one group per line number.

fixes #952

Some signs would not be removed from the sign column when their
diagnostic was fixed.

Debugging revealed that signs_to_add would include two signs for the
same line. `state.signs`is keyed by line number so the second sign for
a line will overwrite the first when signs_to_add is recorded in state.
The first sign's id is lost when it's overwritten so it can't be
removed.

signs_to_add included two signs for the same line because itertools
`group_by` only groups consecutive runs (it's more efficient, you only
need to remember the current group and can flush it when the key
changes).

This quick fix sorts diagnostics by line number so that group_by gives
us one group per line number.

fixes autozimu#952
@everett1992
Copy link
Author

There's a couple ways to fix this issue, I chose the easiest, sorting the iterator so group_by works.

I think it would be faster/efficient-er to create a hash map of line number -> severity then insert each diagnostic into the map if it's severity is less than the current value at the line number.

Overall I think that state mapping by line number is a bad idea, unless you need to index by line number somewhere else. It should index by the sign id. Then if you accidentally add two signs on the same line you'll have two signs and know both id's instead of two signs and only know one id.

@autozimu autozimu merged commit 51fa22f into autozimu:next Jan 21, 2020
@autozimu
Copy link
Owner

Thanks!

@jimmycuadra
Copy link

Was this supposed to handle both warnings and errors? After pulling down the latest commits it seems to have resolved the problem for the latter but not the former.

@everett1992
Copy link
Author

everett1992 commented Jan 22, 2020 via email

@everett1992
Copy link
Author

everett1992 commented Jan 22, 2020 via email

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.

signcolumn includes symbols where no diagnostic exists (signcolumn/gutter is stale)

3 participants