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

Add expand_template rule #330

Merged
merged 1 commit into from
Apr 1, 2022
Merged

Add expand_template rule #330

merged 1 commit into from
Apr 1, 2022

Conversation

Vertexwahn
Copy link
Contributor

@Vertexwahn Vertexwahn commented Nov 1, 2021

@google-cla google-cla bot added the cla: yes label Nov 1, 2021
@Vertexwahn Vertexwahn changed the title Draft: Add expand_template rule Add expand_template rule Nov 1, 2021
Copy link
Collaborator

@tetromino tetromino left a comment

Choose a reason for hiding this comment

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

Overall, looks reasonable; but please add a test for the rule.

rules/expand_template.bzl Outdated Show resolved Hide resolved
rules/expand_template.bzl Show resolved Hide resolved
@Vertexwahn
Copy link
Contributor Author

Vertexwahn commented Mar 11, 2022

@brandjon @tetromino Can you review it again, please? I need expand_template for the bazelization of Intel Embree, Catch2 and OpenEXR. Should I squash the commits? or is this done on merge?

@Vertexwahn
Copy link
Contributor Author

I squashed the commits and rebased the PR

@comius @hvadehra @c-mita Can you please review this PR?

@brandjon brandjon removed their request for review March 21, 2022 20:39
@Vertexwahn Vertexwahn mentioned this pull request Mar 22, 2022
@alexeagle
Copy link
Contributor

Hey @Vertexwahn if you can't get a response here, we'd love to have your contribution in https://github.com/aspect-build/bazel-lib instead!

@Vertexwahn
Copy link
Contributor Author

@alexeagle If this gets not merged I will open up a PR on bazel-lib the next days.

@Vertexwahn
Copy link
Contributor Author

Vertexwahn commented Mar 30, 2022

@tetromino Sorry to ping you again - but you reviewed the PR already in the past, I applied several changes (added tests, etc). Can you please review again?

@comius comius requested review from tetromino and removed request for comius, hvadehra and c-mita March 31, 2022 10:01
Copy link
Collaborator

@tetromino tetromino left a comment

Choose a reason for hiding this comment

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

My apologies for the delayed review!

Tests look good; what remains is docs.

Please add a bzl_library entry for expand_template to rules/BUILD.

Please add a stardoc_with_diff_test target in docs/BUILD which uses that bzl_library.

Then use bazel run //docs:update to create the expand_template_doc.md file, and add a link to the doc in README.md

rules/expand_template.bzl Outdated Show resolved Hide resolved
Add a basic expand_template rule to support trivial search and replace use cases.
@Vertexwahn
Copy link
Contributor Author

@tetromino I addressed all your comments. Looking forward to a merge ;)

Copy link
Collaborator

@tetromino tetromino left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you, and again, sorry for delayed review!

@tetromino tetromino merged commit 2a87d4a into bazelbuild:main Apr 1, 2022
tetromino added a commit to tetromino/bazel-skylib that referenced this pull request Apr 1, 2022
Followup to bazelbuild#330: remove the wrapper macro and export the rule directly; the macro
does not serve any useful function. As a side effect, this fixes the inability to
set tags etc., since the macro did not support **kwargs.
tetromino added a commit that referenced this pull request Apr 6, 2022
Followup to #330: remove the wrapper macro and export the rule directly; the macro
does not serve any useful function. As a side effect, this fixes the inability to
set tags etc., since the macro did not support **kwargs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand template rule
3 participants