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

Quick Fix Import Button #12051

Merged
merged 8 commits into from
Jan 20, 2025
Merged

Quick Fix Import Button #12051

merged 8 commits into from
Jan 20, 2025

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jan 14, 2025

Pull Request Description

Based on the ideas presented in the #12019 discussion this PR introduces a quick fix button capable of automatically adding the missing import statement when a fully qualified name is used without a library being imported.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    TypeScript,
  • Manually tested
  • Unit tests skipped for now

Copy link

github-actions bot commented Jan 14, 2025

🧪 Storybook is successfully deployed!

📊 Dashboard:

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Jan 14, 2025

Let's start with following source code:

from Standard.Base import all

main =
    file1 = Standard.Table.Row.Row
    file2 = Row
    node1 = Standard.Microsoft.Main

by default all three nodes yield an error:

Missing imports

The Row node shows the regular error message as present these days, because the message isn't recognized as something that could be fixed.

However there is another Standard.Table.Row.Row node and it offers a quick fix button (instead of the copy message). Let's click it:

One import added

The source code gets a new import Standard.Table and the Standard.Table.Row.Row node immediately resolves to a node without error. The same can be repeated with the Standard.Microsoft.Main node. After clicking its Fix Import button, the node gets fixed. The final source code looks like this:

from Standard.Base import all
import Standard.Table
import Standard.Microsoft

main =
    file1 = Standard.Table.Row.Row
    file2 = Row
    node1 = Standard.Microsoft.Main


const props = defineProps<{
message: string
type: MessageType
}>()

function containsLibraryName(): string | null {
const prefix = 'Compile error: Fully qualified name references a library '
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this message somewhere as constant? Seems a bit dangerous to hardcode it like this.

Copy link
Member Author

@JaroslavTulach JaroslavTulach Jan 14, 2025

Choose a reason for hiding this comment

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

It may be a constant, but in Java code. We don't have a way to share constants between Java and JavaScript. However you are right. This is slightly fragile, in the long run we could try:

Copy link
Member Author

@JaroslavTulach JaroslavTulach Jan 14, 2025

Choose a reason for hiding this comment

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

Let's enhance the language server protocol we have. For example let's extend Diagnostic with possible hints, like:

hint: {
  type: "import",
  add: "Standard.Table"
}

that way we would formalize this API - would be better in the long run then depending on the text of the message.

@enso-bot enso-bot bot mentioned this pull request Jan 15, 2025
@JaroslavTulach JaroslavTulach added the CI: Keep up to date Automatically update this PR to the latest develop. label Jan 20, 2025
@JaroslavTulach JaroslavTulach added CI: Clean build required CI runners will be cleaned before and after this PR is built. and removed CI: Keep up to date Automatically update this PR to the latest develop. labels Jan 20, 2025
@JaroslavTulach
Copy link
Member Author

The fix import button continues to "fix import" even when latest develop branch was merged in.

  • I think have addressed review comments
  • I haven't written any unit test - I am not sure what to copy...

Can I merge? Before today's demo?

@JaroslavTulach JaroslavTulach changed the title Quick Fix Import button Quick Fix Import Button Jan 20, 2025
@farmaazon
Copy link
Contributor

For unit testing, the code needs to be a bit refactored... But I think they're not needed, as this is easy party and will be overhauled at some point (to not use hard-coded message, for example).

But before merging, see my comment

@JaroslavTulach
Copy link
Member Author

Green enough. Let's get this in and claim a cooperation success.

@JaroslavTulach JaroslavTulach merged commit 17dcbd6 into develop Jan 20, 2025
39 of 40 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/FixImport12019 branch January 20, 2025 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-gui CI: Clean build required CI runners will be cleaned before and after this PR is built.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants