Golang: Display Go external libraries based on importpath structure #2973

blico · 2021-09-30T23:17:09Z

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: #1744 #2365

Description of this change

Modifies the structure of the external "Go Libraries" project view to be based on importpath rather than a flat list of files. See my comment within #2365 for an example of how the new structure looks.

This is essentially achieved by extending Go's additionalLibraryRootsProvider to carry external dependency importpath mappings and then using that information from a new Go treeStructureProvider to create the importpath based directory structure.

Due to my unfamiliarity with this project and the lack of pre-existing tests I struggled to write tests for this feature. If anyone more familiar with the codebase could provide some guidance that would be very helpful.

AlexeyGy · 2021-11-10T15:33:53Z

Thank you very much for this PR and sorry that it took me a while to review!

Unfortunately, I don't think that we can upstream the changes as-is. In particular, it worries me to let state leak out out of ExternalLibraryManager in the form of a importpathToFilesMap. We heavily rely on controlling the state of libraries and sources in our plugin. For example, this state controls what libraries are attached after the sync (this is why "Go external libraries" will initially disappear when you reopen a project). I fear that changing this may cause unforeseeable problems like libraries not disappearing after removing them from BUILD files.

TreeStructureprovider is the right way to go since changes there will only be cosmetic for the user. We can get the current project state via BlazeProjectDataManager.getInstance(project).getProjectData or even via using the SyncCache.

Then, we only need to use the DFS that you've already drafted up to create the right structure.

Testing

In terms of tests, I think, the ideal point would be a unit test which checks if the tree structure is properly generated (i.e., a test for different scenarios ofcreateSyntheticDirectoryStructure) and that covers several bad scenarios (e.g., what if we don't have all entries in importPath->Files map).

How to move ahead

Let me move ahead with drafting up a PR based on this and then you can tell me if it would work for you (let's flip the script). I'd make it so you are co-author of the change on GitHub when it gets submitted. I'd probably also want to have this change behind a feature flag, just to be sure that users who don't want this can fall back on something.

blico · 2021-11-10T16:00:18Z

That makes sense about not wanting to leak state and sounds like a good plan forward. Thank you @AlexeyGy !

blico · 2022-03-22T15:07:02Z

@AlexeyGy @alice-ks friendly ping. Do you have any plans on taking this PR?

AlexeyGy · 2022-03-26T10:32:55Z

@AlexeyGy @alice-ks friendly ping. Do you have any plans on taking this PR?

Still on it and have a WIP PR but did not yet get to push it out mainly because I want this functionality to be well covered by unit tests.

loeffel-io · 2022-06-09T13:08:07Z

Any update?

loeffel-io · 2022-07-21T11:44:06Z

Can someone please merge this thing - it's so annoying

linzhp · 2022-07-21T15:24:45Z

@jastice FYI

jastice · 2022-08-02T12:03:29Z

@AlexeyGy maybe you could publish your draft PR so that somoebody could pick it up?

loeffel-io · 2022-08-02T12:37:03Z

Never wanted a PR so bad ❤️ @linzhp

…bazelbuild/intellij PR import #2973) This is a modified version of the original change by Brandon Lico with a few modifications: * read from the fileToImportPathMap directly when modifying the structure * add unit tests. ## Design choices: * We don't inherit from BlazeExternalSyntheticLibrary since that contains irrelevant logic. Also we can't logically fulfill the "Only supports one instance per value of presentableText." condition that's outlined in the class javadoc since we could have nodes with identical presentableText properties in the tree in different positions. ## Discussion thread for this change Issue number: #1744 #2365 ## Description of this change Modifies the structure of the external "Go Libraries" project view to be based on importpath rather than a flat list of files. See my comment within #2365 for an example of how the new structure looks. Closes #2973 ## Tests Unit tests added. PiperOrigin-RevId: 401216309

AlexeyGy · 2022-08-09T20:02:32Z

New PR is finally out: #3833. Under internal review.

loeffel-io · 2022-08-10T08:05:06Z

🎉

…bazelbuild/intellij PR import #2973) This is a modified version of the original change by Brandon Lico with a few modifications: * read from the fileToImportPathMap directly when modifying the structure * add unit tests. ## Design choices: * We don't inherit from BlazeExternalSyntheticLibrary since that contains irrelevant logic. Also we can't logically fulfill the "Only supports one instance per value of presentableText." condition that's outlined in the class javadoc since we could have nodes with identical presentableText properties in the tree in different positions. ## Discussion thread for this change Issue number: #1744 #2365 ## Description of this change Modifies the structure of the external "Go Libraries" project view to be based on importpath rather than a flat list of files. See my comment within #2365 for an example of how the new structure looks. Closes #2973 ## Tests Unit tests added. PiperOrigin-RevId: 401216309

…bazelbuild/intellij PR import #2973) This is a modified version of the original change by Brandon Lico with a few modifications: * read from the fileToImportPathMap directly when modifying the structure * add unit tests. ## Design choices: * We don't inherit from BlazeExternalSyntheticLibrary since that contains irrelevant logic. Also we can't logically fulfill the "Only supports one instance per value of presentableText." condition that's outlined in the class javadoc since we could have nodes with identical presentableText properties in the tree in different positions. ## Discussion thread for this change Issue number: #1744 #2365 ## Description of this change Modifies the structure of the external "Go Libraries" project view to be based on importpath rather than a flat list of files. See my comment within #2365 for an example of how the new structure looks. Closes #2973 ## Tests Unit tests added. PiperOrigin-RevId: 467145600

See #2365 and #2973 for context. PiperOrigin-RevId: 467146104

See #2365 and #2973 for context. PiperOrigin-RevId: 467183336

tpasternak · 2022-08-12T13:04:00Z

@blico once the change is merged, can we close this PR?

blico · 2022-08-12T18:22:11Z

Done!

linzhp · 2022-08-12T18:45:05Z

@AlexeyGy In the future, it's a better to separate your change from @blico's in different commits, so the original author get some credits in the list of commits

…bazelbuild/intellij PR import #2973) This is a modified version of the original change by Brandon Lico with a few modifications: * read from the fileToImportPathMap directly when modifying the structure * add unit tests. * We don't inherit from BlazeExternalSyntheticLibrary since that contains irrelevant logic. Also we can't logically fulfill the "Only supports one instance per value of presentableText." condition that's outlined in the class javadoc since we could have nodes with identical presentableText properties in the tree in different positions. Issue number: #1744 #2365 Modifies the structure of the external "Go Libraries" project view to be based on importpath rather than a flat list of files. See my comment within #2365 for an example of how the new structure looks. Closes #2973 Unit tests added. (cherry picked from commit 4d0f56b)

See #2365 and #2973 for context. (cherry picked from commit 0194efa)

…bazelbuild/intellij PR import bazelbuild#2973) This is a modified version of the original change by Brandon Lico with a few modifications: * read from the fileToImportPathMap directly when modifying the structure * add unit tests. * We don't inherit from BlazeExternalSyntheticLibrary since that contains irrelevant logic. Also we can't logically fulfill the "Only supports one instance per value of presentableText." condition that's outlined in the class javadoc since we could have nodes with identical presentableText properties in the tree in different positions. Issue number: bazelbuild#1744 bazelbuild#2365 Modifies the structure of the external "Go Libraries" project view to be based on importpath rather than a flat list of files. See my comment within bazelbuild#2365 for an example of how the new structure looks. Closes bazelbuild#2973 Unit tests added. (cherry picked from commit 4d0f56b)

See bazelbuild#2365 and bazelbuild#2973 for context. (cherry picked from commit 0194efa)

See #2365 and #2973 for context. Co-authored-by: Brandon Lico <[email protected]>

…bazelbuild/intellij PR import bazelbuild#2973) This is a modified version of the original change by Brandon Lico with a few modifications: * read from the fileToImportPathMap directly when modifying the structure * add unit tests. * We don't inherit from BlazeExternalSyntheticLibrary since that contains irrelevant logic. Also we can't logically fulfill the "Only supports one instance per value of presentableText." condition that's outlined in the class javadoc since we could have nodes with identical presentableText properties in the tree in different positions. Issue number: bazelbuild#1744 bazelbuild#2365 Modifies the structure of the external "Go Libraries" project view to be based on importpath rather than a flat list of files. See my comment within bazelbuild#2365 for an example of how the new structure looks. Closes bazelbuild#2973 Unit tests added. (cherry picked from commit 4d0f56b)

See bazelbuild#2365 and bazelbuild#2973 for context. (cherry picked from commit 0194efa)

importpath based structure

52c4a99

blico requested review from alice-ks and vsiva as code owners September 30, 2021 23:17

google-cla bot added the cla: yes label Sep 30, 2021

linzhp mentioned this pull request May 9, 2022

Expose generated Go files to editors and IDEs bazelbuild/rules_go#512

Closed

jastice requested review from jastice, mai93 and tpasternak and removed request for vsiva and alice-ks July 21, 2022 15:26

copybara-service bot mentioned this pull request Aug 9, 2022

Golang: Display Go external libraries based on importpath structure (bazelbuild/intellij PR import #2973) #3833

Merged

copybara-service bot pushed a commit that referenced this pull request Aug 12, 2022

Rollout: Display Go external libraries based on the import-path

ef21c6e

See #2365 and #2973 for context. PiperOrigin-RevId: 467146104

copybara-service bot mentioned this pull request Aug 12, 2022

Rollout: Display Go external libraries based on the import-path #3836

Merged

copybara-service bot pushed a commit that referenced this pull request Aug 12, 2022

Rollout: Display Go external libraries based on the import-path

b348013

See #2365 and #2973 for context. PiperOrigin-RevId: 467146104

copybara-service bot pushed a commit that referenced this pull request Aug 12, 2022

Rollout: Display Go external libraries based on the import-path

38776ba

See #2365 and #2973 for context. PiperOrigin-RevId: 467146104

copybara-service bot pushed a commit that referenced this pull request Aug 12, 2022

Rollout: Display Go external libraries based on the import-path

0194efa

See #2365 and #2973 for context. PiperOrigin-RevId: 467183336

blico closed this Aug 12, 2022

mai93 pushed a commit that referenced this pull request Aug 13, 2022

Rollout: Display Go external libraries based on the import-path

7306575

See #2365 and #2973 for context. (cherry picked from commit 0194efa)

cpsauer pushed a commit to hedronvision/bazelbuild-intellij that referenced this pull request Aug 13, 2022

Rollout: Display Go external libraries based on the import-path

5dc9325

See bazelbuild#2365 and bazelbuild#2973 for context. (cherry picked from commit 0194efa)

AlexeyGy added a commit that referenced this pull request Aug 16, 2022

Move go.import.treestructure to 100%

18ae5b5

See #2365 and #2973 for context. Co-authored-by: Brandon Lico <[email protected]>

AlexeyGy mentioned this pull request Aug 16, 2022

Move go.import.treestructure to 100% #3848

Closed

ittaiz pushed a commit to wix-playground/intellij that referenced this pull request Oct 24, 2022

Rollout: Display Go external libraries based on the import-path

84a3dcb

See bazelbuild#2365 and bazelbuild#2973 for context. (cherry picked from commit 0194efa)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Golang: Display Go external libraries based on importpath structure #2973

Golang: Display Go external libraries based on importpath structure #2973

blico commented Sep 30, 2021 •

edited

Loading

AlexeyGy commented Nov 10, 2021

blico commented Nov 10, 2021

blico commented Mar 22, 2022

AlexeyGy commented Mar 26, 2022

loeffel-io commented Jun 9, 2022

loeffel-io commented Jul 21, 2022

linzhp commented Jul 21, 2022

jastice commented Aug 2, 2022

loeffel-io commented Aug 2, 2022

AlexeyGy commented Aug 9, 2022

loeffel-io commented Aug 10, 2022

tpasternak commented Aug 12, 2022

blico commented Aug 12, 2022

linzhp commented Aug 12, 2022

Golang: Display Go external libraries based on importpath structure #2973

Golang: Display Go external libraries based on importpath structure #2973

Conversation

blico commented Sep 30, 2021 • edited Loading

Checklist

Discussion thread for this change

Description of this change

AlexeyGy commented Nov 10, 2021

Testing

How to move ahead

blico commented Nov 10, 2021

blico commented Mar 22, 2022

AlexeyGy commented Mar 26, 2022

loeffel-io commented Jun 9, 2022

loeffel-io commented Jul 21, 2022

linzhp commented Jul 21, 2022

jastice commented Aug 2, 2022

loeffel-io commented Aug 2, 2022

AlexeyGy commented Aug 9, 2022

loeffel-io commented Aug 10, 2022

tpasternak commented Aug 12, 2022

blico commented Aug 12, 2022

linzhp commented Aug 12, 2022

blico commented Sep 30, 2021 •

edited

Loading