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.

Sign up for GitHub

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

Jump to bottom

Add 'View project root' flag #6422

Merged
merged 4 commits into from
May 13, 2024
Merged

Add 'View project root' flag #6422

merged 4 commits into from
May 13, 2024

Conversation

tpasternak
Copy link
Contributor

@tpasternak tpasternak commented May 7, 2024 •
edited
Loading

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.

Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.

Discussion thread for this change

Issue number: <please reference the issue number or url here>

Description of this change

Screen.Recording.2024-05-07.at.15.14.12.mov

@tpasternak tpasternak force-pushed the view-project-root branch from aec3ccc to 16a2cd7 Compare May 8, 2024 17:25
@tpasternak tpasternak force-pushed the view-project-root branch from 16a2cd7 to 0af924b Compare May 8, 2024 17:42
@tpasternak tpasternak marked this pull request as ready for review May 8, 2024 17:46
@github-actions github-actions bot added product: CLion CLion plugin product: IntelliJ IntelliJ plugin product: GoLand GoLand plugin awaiting-review Awaiting review from Bazel team on PRs labels May 8, 2024
@tpasternak tpasternak requested review from blorente and removed request for jastice and agluszak May 8, 2024 17:46
blorente
blorente approved these changes May 10, 2024
View reviewed changes
Copy link
Collaborator

@blorente blorente left a comment

Choose a reason for hiding this comment

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

Approved with very minor comments! Thanks a lot for this change.

base/src/com/google/idea/blaze/base/projectview/section/sections/ViewProjectRootSection.java Outdated
import com.google.idea.blaze.base.projectview.section.SectionKey;
import com.google.idea.blaze.base.projectview.section.SectionParser;

/** If set to true, automatically derives targets from the project directories. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this comment is out of date.

base/tests/unittests/com/google/idea/blaze/base/projectview/ProjectViewSetTest.java
@@ -120,6 +121,7 @@ public void testProjectViewSetSerializable() {
ScalarSection.builder(BuildConfigSection.KEY)
.set(new WorkspacePath("test")))
.add(ScalarSection.builder(UseExclusionPatternsSection.KEY).set(false))
.add(ScalarSection.builder(ViewProjectRootSection.KEY).set(false))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a test where we actually set it in a project view? We don't have to assert anything on the actual behaviour, I just want to check that it parses.

Update ViewProjectRootSection.java
f646fa8 
Remove comment for ViewProjectRootSection

It's  already commented in the PARSER definition
@tpasternak
Copy link
Contributor Author

Oh, I can't just test it properly with the current test framework, because it converts TestFileSystem's files to NIO files via paths without hestitation, so as a result the tests would crash because it's impossible to call listFiles on it. I removed the comment (as it's already commented in string) and I'll try to think about fixing the test framework later

https://github.com/bazelbuild/intellij/blob/master/base/tests/utils/integration/com/google/idea/blaze/base/BlazeIntegrationTestCase.java#L110

@tpasternak tpasternak requested a review from blorente May 13, 2024 10:14
@blorente
Copy link
Collaborator

That's fair, it's fine to leave the proper tests as a follow up. Do you think it would be hard to add a test that asserts that the section parses correctly? So a test that takes the string:

view_project_root: true

And asserts that the right Section is created.

@tpasternak
Copy link
Contributor Author

Ok, that's easy, could you take a look again?

@blorente
Copy link
Collaborator

Yeah, looks good to me! Thanks for this change.

@tpasternak tpasternak merged commit 4c3dda8 into bazelbuild:master May 13, 2024
6 checks passed
@tpasternak tpasternak deleted the view-project-root branch May 13, 2024 16:30
@github-actions github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label May 13, 2024
@mai93 mai93 mentioned this pull request May 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mai93 mai93 Awaiting requested review from mai93 mai93 is a code owner

@blorente blorente Awaiting requested review from blorente

Assignees

@tpasternak tpasternak

@blorente blorente

@mai93 mai93

Labels
product: CLion CLion plugin product: GoLand GoLand plugin product: IntelliJ IntelliJ plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tpasternak @blorente @mai93