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

Do not suggest to add .ijwb and others to project view. #5168

Merged
merged 6 commits into from
Aug 7, 2023
Merged

Do not suggest to add .ijwb and others to project view. #5168

merged 6 commits into from
Aug 7, 2023

Conversation

dkashyn-sfdc
Copy link
Contributor

@dkashyn-sfdc dkashyn-sfdc commented Jul 30, 2023 •
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

The following suggestion doesn't make much sense
image
since it results in

(14:21:46) Loading: 
(14:21:46) Loading: 
(14:21:46) Loading: 0 packages loaded
(14:21:46) ERROR: Skipping '//.ijwb/...:all': no targets found beneath '.ijwb'
(14:21:46) WARNING: Target pattern parsing failed.
(14:21:46) Analyzing: 0 targets (0 packages loaded, 0 targets configured)
(14:21:46) INFO: Analyzed 0 targets (0 packages loaded, 0 targets configured).
(14:21:46) INFO: Found 0 targets...
(14:21:47) [0 / 1] [Prepa] BazelWorkspaceStatusAction stable-status.txt
(14:21:47) ERROR: command succeeded, but there were errors parsing the target pattern
(14:21:47) INFO: Elapsed time: 3.584s, Critical Path: 0.00s

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

Description of this change

Condition added to not render the suggestion to add a file to the project if the file is located somewhere in .ijwb or in a similar folder for other products.

@sgowroji sgowroji added product: IntelliJ IntelliJ plugin awaiting-review Awaiting review from Bazel team on PRs labels Jul 31, 2023
blorente
blorente reviewed Jul 31, 2023
View reviewed changes
base/src/com/google/idea/blaze/base/dependencies/ExternalFileProjectManagementHelper.java Outdated
if (alreadyBuilt && inProjectDirectories) {
// We do not want to add `/.ijwb` to the project view since it has no BUILD files
// This helps to avoid `ERROR: Skipping '//.ijwb/...:all': no targets found beneath '.ijwb'`
if (context.file.getPath().contains(String.format("/%s/", BlazeDataStorage.PROJECT_DATA_SUBDIRECTORY))
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could use BlazeDataStorage.getAllProjectSubdirectories() to account for .clwb and .aswb

Copy link
Contributor Author

@dkashyn-sfdc dkashyn-sfdc Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

public static final String PROJECT_DATA_SUBDIRECTORY = getProjectDataSubdirectory(); seems already using it and private static String getProjectDataSubdirectory() is not visible from the outside.

image

Copy link
Collaborator

@blorente blorente Aug 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It does, but it will only exclude the one in the IDE you're using. Say someone has some Java and some C++ in the project. They may have to open both with IntelliJ and CLion at the same time, so we have to ignore all of them from every IDE.

I have opened a PR to this PR updating the code to expose an ALL_PROJECT_SUBDIRECTORIES constant. Feel free to review it and merge it if useful: dkashyn-sfdc#1

@mai93 mai93 assigned blorente and unassigned mai93 Jul 31, 2023
@dkashyn-sfdc
Copy link
Contributor Author

@blorente could you please help with the review?

blorente
blorente reviewed Aug 4, 2023
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.

Thanks for the reply! I've left a follow-up comment.

@dkashyn-sfdc
Copy link
Contributor Author

@blorente updated according to your recommendations, could you please check?

blorente
blorente approved these changes Aug 7, 2023
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.

Thanks for the changes! LGTM.

@blorente blorente merged commit 1bec01e into bazelbuild:master Aug 7, 2023
1 check passed
@github-actions github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label Aug 7, 2023
@mai93 mai93 mentioned this pull request Aug 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blorente blorente blorente approved these changes

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

@jastice jastice Awaiting requested review from jastice

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

Assignees

@blorente blorente

Labels
product: IntelliJ IntelliJ plugin
Projects
Bazel IntelliJ Plugin
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dkashyn-sfdc @blorente @sgowroji @mai93 @victoriaroseb