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

[#6664] Resolve a workspace label across external repositories - 4/n #6700

Conversation

mtoader
Copy link
Contributor

@mtoader mtoader commented Aug 30, 2024

Checklist

  • I have filed an issue about this change and discussed potential changes with the maintainers.
  • I have received the approval from the maintainers to make this change.
  • This is not a stylistic, refactoring, or cleanup change.

Discussion thread for this change

Issue number: #6664 (#6664)

Description of this change

This enabled LabelReference to resolve if pointed inside a module-defined external repository. It will not do that recursively yet, and it doesn't complete yet.

Added some extensions to the BuildFileIntegrationTestCase to enable testing these scenarios and added two tests (one for non renamed external workspace and one for an aliased external workspace).

Resolving with Cmd + click
image

and the target:
image

@mtoader mtoader force-pushed the mtoader/resolve-a-workspace-label-across-external-repositories branch from 6ed571f to b325547 Compare August 30, 2024 22:07
@mtoader mtoader marked this pull request as draft September 1, 2024 21:46
@mtoader
Copy link
Contributor Author

mtoader commented Sep 1, 2024

Making this a draft since I need to add some changes to it once the #6704 lands

@mtoader mtoader marked this pull request as ready for review September 3, 2024 16:32
@mtoader
Copy link
Contributor Author

mtoader commented Sep 4, 2024

@tpasternak This is ready btw.

WorkspaceFileSystem workspaceFileSystem;

public ExternalWorkspaceFixture(ExternalWorkspace w, TestFileSystem fileSystem) {
this.w = w;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's change the name (in a follow up PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What name do you think would work here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean the w or the fixture name ?

)""", targetLabel));

Editor editor = editorTest.openFileInEditor(file);
editorTest.setCaretPosition(editor, 2, (" deps = ['" + targetLabel).length());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test relies on whitespace length, etc. I would prefer to extract the whole line to the variable to avoid duplication of strings here. Can be done in a follow-up PR ofc

Copy link
Contributor Author

@mtoader mtoader Sep 4, 2024

Choose a reason for hiding this comment

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

The next PR in the chain switches to something similar to configureByText("asdfasf<caret>") which is more useful. I can try to put that in here too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, ignore then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a change to use configureByText

@tpasternak
Copy link
Collaborator

I still couldn't get the navigation working, because for some reason I'm getting null here, however the progress is impressive, thank you @mtoader

PsiFile psiFile = PsiManager.getInstance(project).findFile(buildFile);

@tpasternak tpasternak merged commit a5299e3 into bazelbuild:master Sep 4, 2024
6 checks passed
@github-actions github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label Sep 4, 2024
@mtoader
Copy link
Contributor Author

mtoader commented Sep 4, 2024

I still couldn't get the navigation working, because for some reason I'm getting null here, however the progress is impressive, thank you @mtoader

PsiFile psiFile = PsiManager.getInstance(project).findFile(buildFile);

You most likely need a Full sync since the whole resolution depends on the repo mapping data being correct and after the last issue with bazel mod only a full sync will fully reset the in-memory state

@mtoader mtoader deleted the mtoader/resolve-a-workspace-label-across-external-repositories branch September 4, 2024 08:30
@tpasternak
Copy link
Collaborator

ok, I've found the root cause #6664 (comment)

@tpasternak
Copy link
Collaborator

cc @ivyspirit you might be interested in this change ☝️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product: CLion CLion plugin product: GoLand GoLand plugin product: IntelliJ IntelliJ plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants