Skip to content

fix(typescript): propagate tags to validate_options#3260

Merged
alexeagle merged 2 commits intobazel-contrib:stablefrom
djmarcin:ts-project-propagate
Jan 18, 2022
Merged

fix(typescript): propagate tags to validate_options#3260
alexeagle merged 2 commits intobazel-contrib:stablefrom
djmarcin:ts-project-propagate

Conversation

@djmarcin
Copy link
Contributor

@djmarcin djmarcin commented Jan 17, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #3259

What is the new behavior?

Tags are propagated to the validate_options rule

Does this PR introduce a breaking change?

  • Yes
  • No

Strictly, the answer here might be yes, particularly if someone was doing something very esoteric with custom tags and relying on the fact that propagation to this rule was not happening, but this seems extremely unlikely.

Other information

alexeagle
alexeagle approved these changes Jan 17, 2022
View reviewed changes
Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Thanks!

Do you think we might want testonly and visibility to be propagated as well? Just thinking to be consistent with https://github.com/bazelbuild/rules_nodejs/blob/5.x/packages/typescript/index.bzl#L507-L511

@djmarcin
Copy link
Contributor Author

djmarcin commented Jan 17, 2022

Thanks!

Do you think we might want testonly and visibility to be propagated as well? Just thinking to be consistent with https://github.com/bazelbuild/rules_nodejs/blob/5.x/packages/typescript/index.bzl#L507-L511

Seems reasonable. The target is named with a leading underscore (_validate...) so it seems like it's not intended to be depended on by other things, making testonly and visibility likely extraneous but it probably doesn't hurt.

@djmarcin
Copy link
Contributor Author

djmarcin commented Jan 18, 2022

@alexeagle I moved common_kwargs up and reused it for validate_options. It seems like this may also be good to pass to write_tsconfig and nodejs_binary, but I did not do that here as it expands the scope of the PR a bit. The write_tsconfig macro needs to be further modified to take & pass along **kwargs to the underlying rule. Let me know if I should add that here.

@alexeagle
Copy link
Collaborator

alexeagle commented Jan 18, 2022

buildifier failed - I'd suggest running pre-commit install in this repo so a git pre-commit hook will always format your starlark.

@djmarcin djmarcin force-pushed the ts-project-propagate branch from a499713 to afc5937 Compare January 18, 2022 00:58
@djmarcin
Copy link
Contributor Author

djmarcin commented Jan 18, 2022

buildifier failed - I'd suggest running pre-commit install in this repo so a git pre-commit hook will always format your starlark.

fixed & force pushed the last commit (not sure what the convention for this repo is in terms of the structured commit messages and trivial typo fixes like that...)

@alexeagle alexeagle merged commit e8b2e8f into bazel-contrib:stable Jan 18, 2022
@djmarcin djmarcin deleted the ts-project-propagate branch January 18, 2022 03:49
alexeagle pushed a commit that referenced this pull request Jan 20, 2022
@djmarcin @alexeagle
fix(typescript): propagate tags to validate_options (#3260)
4615198
@mattem mattem mentioned this pull request Feb 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexeagle alexeagle alexeagle approved these changes

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

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

@soldair soldair Awaiting requested review from soldair

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@djmarcin @alexeagle