Allow to have non-existent import entries in `projectview` that pointing to … #5689

dkashyn-sfdc · 2023-11-14T01:22:26Z

…files that are optional and can be missing for some of the users.

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: 5688

Description of this change

…files that are optional and can be missing for some of the users.

dkashyn-sfdc · 2023-11-14T01:23:57Z

base/tests/unittests/com/google/idea/blaze/base/projectview/parser/ProjectViewParserTest.java

@@ -303,11 +306,26 @@ public void testCanParseWithMissingCarriageReturnAtEndOfSection() {

  @Test
  public void testImportMissingFileResultsInIssue() {
+    Registry.get("bazel.projectview.optional.imports").setValue(false);


I was unable to make it work with Registry.is('bla', false) for tests.

dkashyn-sfdc · 2023-11-14T01:24:15Z

base/tests/utils/unit/com/google/idea/blaze/base/scope/ErrorCollector.java

@@ -55,6 +55,10 @@ public void assertHasErrors() {
    assertThat(issues.stream().anyMatch(i -> i.getCategory() == Category.ERROR)).isTrue();
  }

+  public void assertHasNoErrors() {


in some cases there are issues but those are not errors.

base/src/com/google/idea/blaze/base/projectview/section/sections/ImportSection.java

ujohnny · 2023-11-14T17:52:51Z

codewise lgtm, haven't checked how it works IRL. @tpasternak could you please check this?

related though: I'd be happy to see warnings (primarily highlighting) in editor on unresolved imports when feature is enabled, though I believe it falls outside of the scope of proposed change.

dkashyn-sfdc · 2023-11-20T22:03:15Z

sigh...

ujohnny

We had a discussion about this feature and the common point was that primarily in all programming languages imports require imported entity to exist. Though for example python allows to handle import errors, so can we keep import statement as is and just introduce try_import which will allow imported entity to be absent?

dkashyn-sfdc · 2023-11-22T15:47:38Z

I don't think that .projectview is a programing language to be that strict :)

dkashyn-sfdc · 2023-11-22T16:09:43Z

@ujohnny please note that this is protected by registry value and not default behavior.

Our use case is following:

we do have templates
customers use those templates to create their projects
template changes over time and we delete/remove imports
files not there anymore and projects generated as part of 2 and oblivious to 3 still expecting files to be there when they are physically not

In our case we want to have freedom to remove any imported file without much deprecation process.
Every import that we have should be try-import based on what you are suggesting.

If we go via try-import way then we don't need that registry key.

Could you please consider approving this PR and if someone needs try-import they can add it in the future.

dkashyn-sfdc · 2023-11-22T16:11:12Z

With your suggestion we need to ask EVERY engineer to replace import in their existing projects with try-import and this is what we wanted to avoid by enabling optional imports with warning without involvement of EVERY engineer who have their project already configured.

ujohnny · 2023-11-29T13:24:08Z

@dkashyn-sfdc you could leave implicit optional import support under the flag, but please then make try_import statement available for everyone.

…ojectview

dkashyn-sfdc · 2023-11-29T18:52:21Z

@ujohnny could you please check now? I'll think of other tests if the approach is correct.

dkashyn-sfdc · 2023-11-29T18:55:02Z

base/src/com/google/idea/blaze/base/projectview/ProjectViewSet.java

@@ -52,10 +52,12 @@ public <T> List<T> listItems(SectionKey<T, ListSection<T>> key) {
  }

  /** Returns all values from all scalar sections in the project views, in order */
-  public <T> List<T> listScalarItems(SectionKey<T, ScalarSection<T>> key) {
+  public <T> List<T> listScalarItems(SectionKey<T, ScalarSection<T>>... keys) {


not sure whether this is a good idea so can redo if someone have strong opinions against this way

dkashyn-sfdc · 2023-11-29T19:13:18Z

Added as try_import as requested but bazelrc has it as try-import https://bazel.build/run/bazelrc

ujohnny

LGTM. @tpasternak would you like to check this?

mai93 · 2023-12-21T07:18:34Z

base/src/com/google/idea/blaze/base/wizard2/ui/BlazeEditProjectViewControl.java

@@ -599,6 +600,9 @@ public void updateBuilder(BlazeNewProjectBuilder builder) {
              .add(
                  ScalarSection.builder(ImportSection.KEY)
                      .set(selectProjectViewOption.getSharedProjectView()))
+              .add(


why is this needed? It results in having both import and try_import sections in the generated local projectview file even with the flag disabled.

It is ok to use both import and try_import. It might be not ok to have some duplication or excessive defaults...

Could you please share more context for your inquiry?

I do not know why it is needed to add a try_import in this function? If I understand correctly, this function creates a project view that imports the shared project view file selected in the import process. So import section should be enough and there is no need to try_import the same project view.

@mai93 there might be "mandatory" and "optional" imports in any project view. One example of optional would be

try_import: ~/.some.local.projectview

(let's assume ~ is resolved to user home dir ;) )

I see, I'm still not sure what the effect of having something like:

import hello/hello.bazelproject try_import hello/hello.bazelproject

will this provide a different result from only

import hello/hello.bazelproject

It is rather for

import hello/fail.if.not.present.bazelproject try_import hello/ok.if.not.present.bazelproject

That can result in:

both imported

only hello/fail.if.not.present.bazelproject imported and accepting the fact that that try_import hello/ok.if.not.present.bazelproject is not there

failing if hello/fail.if.not.present.bazelproject is missing

Optional imports will allow engineers to have pre-defined custom import files both present or missing.

Now it is possible for some of the users to benefit from custom contents while other users not affected even if they have no local customizations. For both groups of users the same .bazelproject can be stored in VCS now.

so there is no additional value for having:

import hello/hello.bazelproject try_import hello/hello.bazelproject

right? If so, then the change in this line can be reverted because it adds try_import section for the same project view file: selectProjectViewOption.getSharedProjectView() that already was added with import in line 600.

Reverting here #5958 @mai93

Allow to have non-existent entries in projectview that pointing to …

a23d29a

…files that are optional and can be missing for some of the users.

dkashyn-sfdc requested review from mai93, jastice, tpasternak and agluszak as code owners November 14, 2023 01:22

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 Nov 14, 2023

github-actions bot assigned blorente, mai93 and tpasternak Nov 14, 2023

dkashyn-sfdc commented Nov 14, 2023

View reviewed changes

ujohnny reviewed Nov 14, 2023

View reviewed changes

base/src/com/google/idea/blaze/base/projectview/section/sections/ImportSection.java Outdated Show resolved Hide resolved

Reordering of conditions for readability

54a1ff2

dkashyn-sfdc mentioned this pull request Nov 14, 2023

Allow to tolerate absence of import files referenced in projectview #5688

Closed

ujohnny requested changes Nov 22, 2023

View reviewed changes

mai93 removed their assignment Nov 22, 2023

Merge branch 'master' into optional-imports-projectview

9215b8a

dkashyn-sfdc added 2 commits November 29, 2023 09:58

Merge remote-tracking branch 'origin/master' into optional-imports-pr…

1fabf5d

…ojectview

TryImport POC

ae15030

dkashyn-sfdc commented Nov 29, 2023

View reviewed changes

try_import instead of import

841a9aa

dkashyn-sfdc added 2 commits November 29, 2023 11:41

Some tests for try_import

f1377b5

test fix

b174bc6

dkashyn-sfdc requested a review from ujohnny December 1, 2023 02:57

ujohnny approved these changes Dec 5, 2023

View reviewed changes

ujohnny merged commit fec469b into bazelbuild:master Dec 11, 2023

github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label Dec 11, 2023

tpasternak mentioned this pull request Dec 18, 2023

Update changelog for 2023.12.19 release #5861

Merged

rakshitasingh05 mentioned this pull request Dec 19, 2023

Update changelog for 2023.12.19 release #5871

Closed

mai93 reviewed Dec 21, 2023

View reviewed changes

mai93 mentioned this pull request Jan 16, 2024

Do not add 2 import sections for the same project view #5950

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to have non-existent import entries in `projectview` that pointing to … #5689

Allow to have non-existent import entries in `projectview` that pointing to … #5689

dkashyn-sfdc commented Nov 14, 2023 •

edited

Loading

dkashyn-sfdc Nov 14, 2023

dkashyn-sfdc Nov 14, 2023

ujohnny commented Nov 14, 2023

dkashyn-sfdc commented Nov 20, 2023

ujohnny left a comment

dkashyn-sfdc commented Nov 22, 2023

dkashyn-sfdc commented Nov 22, 2023

dkashyn-sfdc commented Nov 22, 2023

ujohnny commented Nov 29, 2023

dkashyn-sfdc commented Nov 29, 2023

dkashyn-sfdc Nov 29, 2023

dkashyn-sfdc commented Nov 29, 2023

ujohnny left a comment

mai93 Dec 21, 2023 •

edited

Loading

dkashyn-sfdc Dec 21, 2023

mai93 Dec 27, 2023

dkashyn-sfdc Dec 27, 2023 •

edited

Loading

mai93 Dec 27, 2023

dkashyn-sfdc Dec 27, 2023

mai93 Dec 27, 2023

dkashyn-sfdc Jan 17, 2024

Allow to have non-existent import entries in projectview that pointing to … #5689

Allow to have non-existent import entries in projectview that pointing to … #5689

Conversation

dkashyn-sfdc commented Nov 14, 2023 • edited Loading

Checklist

Discussion thread for this change

Description of this change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujohnny commented Nov 14, 2023

dkashyn-sfdc commented Nov 20, 2023

ujohnny left a comment

Choose a reason for hiding this comment

dkashyn-sfdc commented Nov 22, 2023

dkashyn-sfdc commented Nov 22, 2023

dkashyn-sfdc commented Nov 22, 2023

ujohnny commented Nov 29, 2023

dkashyn-sfdc commented Nov 29, 2023

Choose a reason for hiding this comment

dkashyn-sfdc commented Nov 29, 2023

ujohnny left a comment

Choose a reason for hiding this comment

mai93 Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkashyn-sfdc Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Allow to have non-existent import entries in `projectview` that pointing to … #5689

Allow to have non-existent import entries in `projectview` that pointing to … #5689

dkashyn-sfdc commented Nov 14, 2023 •

edited

Loading

mai93 Dec 21, 2023 •

edited

Loading

dkashyn-sfdc Dec 27, 2023 •

edited

Loading