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

Allow manual tags to be synced to the project #5085

Merged
merged 3 commits into from
Jul 27, 2023

Conversation

rogerhu
Copy link
Contributor

@rogerhu rogerhu commented Jul 12, 2023

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: #4546

Description of this change

Adds a allow_manual_targets_sync: true in the .bazelproject to allow manual tags.

@rogerhu rogerhu changed the title First pass at allowing manual syncs Allow manual tags to be synced to the project Jul 12, 2023
@rogerhu rogerhu marked this pull request as ready for review July 12, 2023 06:48
@Pavank1992 Pavank1992 added awaiting-review Awaiting review from Bazel team on PRs product: IntelliJ IntelliJ plugin labels Jul 12, 2023
Copy link
Collaborator

@tpasternak tpasternak left a comment

Choose a reason for hiding this comment

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

@rogerhu I'm not sure, but it seems like it would be possible to add a test, a similar to this one

We could set "derive_targets_from_directories: true" there and then cover both cases when the new flag is set, and when it is not

@rogerhu
Copy link
Contributor Author

rogerhu commented Jul 25, 2023

@rogerhu I'm not sure, but it seems like it would be possible to add a test, a similar to this one

We could set "derive_targets_from_directories: true" there and then cover both cases when the new flag is set, and when it is not

I added an integration test: c63ca48

Can you review?

@rogerhu rogerhu force-pushed the rogerh/ISSUE-4546-2 branch 2 times, most recently from 81301b5 to c63ca48 Compare July 25, 2023 23:34
Comment on lines 73 to 91
static class DefaultValueProvider implements ProjectViewDefaultValueProvider {
@Override
public ProjectView addProjectViewDefaultValue(
BuildSystemName buildSystemName,
ProjectViewSet projectViewSet,
ProjectView topLevelProjectView) {
if (!topLevelProjectView.getSectionsOfType(KEY).isEmpty()) {
return topLevelProjectView;
}
return ProjectView.builder(topLevelProjectView)
.add(
TextBlockSection.of(
TextBlock.of(
"# Automatically targets tagged as manual to be synced")))
.add(ScalarSection.builder(KEY).set(false))
.add(TextBlockSection.of(TextBlock.newLine()))
.build();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes the flag visible in the default project view generated by IntelliJ. In general, we have many flags, but we put only the most important ones to the default project view. How about removing this section at all, and keep the flag hidden in the default project view? Apart from this, I think we can merge it

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed for now! Any suggestions for what docs to modify?

Copy link
Collaborator

@tpasternak tpasternak left a comment

Choose a reason for hiding this comment

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

Thank you @rogerhu!

@tpasternak tpasternak merged commit a57b5d4 into bazelbuild:master Jul 27, 2023
1 check passed
return false;
}
parseContext.addError(
"'allow_manual_tags_sync' must be set to 'true' or 'false' (e.g."
Copy link
Contributor

Choose a reason for hiding this comment

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

It supposed to use allow_manual_targets_sync probably?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep fixed here: #5197

@dkashyn-sfdc
Copy link
Contributor

dkashyn-sfdc commented Nov 2, 2023

@rogerhu @tpasternak Can we have this documented somewhere next to https://ij.bazel.build/docs/project-views.html#derive_targets_from_directories ? I'm keep sharing the issue for this PR, when our engineers asking what this tag even is, but I think docs supposed to reflect such properties.

@rogerhu
Copy link
Contributor Author

rogerhu commented Nov 2, 2023

Filed a ticket #5590 since apparently the docs are not hosted in open source land. =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review Awaiting review from Bazel team on PRs product: IntelliJ IntelliJ plugin
Projects
Development

Successfully merging this pull request may close these issues.

4 participants