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

Replace Diagnostic by a TypedDict #1533

Merged
merged 4 commits into from
Dec 30, 2020

Conversation

rwols
Copy link
Member

@rwols rwols commented Dec 23, 2020

In preparation for #1527, simplify a lot of the code.

Replacing Diagnostic by a TypedDict made me rethink the "next/prev diagnostic" functionality. I'm convinced it can just as well be done with the existing popup code using ST's built-in F4 (next_result) and shift+F4 (prev_result). This has the major advantage of having much less code to maintain. As well as not having to account for two different ways to render a diagnostic to minihtml.

It's quite a bit of changes. If you want to inspect it I'd suggest looking at the two commits separately as they should be stand-alone.

@rchl
Copy link
Member

rchl commented Dec 25, 2020

Haven't looked at the changes but have a question. Are your changes compatible with the use-case where the user doesn't use the diagnostic panel but just wants to navigate the diagnostics shown in the view?

You said that you are relying on native next/previous_result but doesn't that require the results to be visible in the panel?

@rwols
Copy link
Member Author

rwols commented Dec 26, 2020

You said that you are relying on native next/previous_result but doesn't that require the results to be visible in the panel?

No. this works even when the panel is not visible :)

@rwols rwols merged commit b84f25e into st4000-exploration Dec 30, 2020
@rwols rwols deleted the refactor/diagnostics-code-actions branch December 30, 2020 18:53
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