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

Validate ts project #1722

Merged
merged 2 commits into from
Mar 26, 2020
Merged

Conversation

alexeagle
Copy link
Collaborator

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: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@alexeagle alexeagle force-pushed the validate_ts_project branch 5 times, most recently from 81ce6a1 to e4bc646 Compare March 25, 2020 00:29
@alexeagle alexeagle force-pushed the validate_ts_project branch from e4bc646 to 6cef733 Compare March 25, 2020 01:03
Copy link
Collaborator

@gregmagolan gregmagolan left a comment

Choose a reason for hiding this comment

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

Nice. This is a beauty.
The only nit I would add is a ts_project test where that is intentionally mismatched with validate = False.

console.error('You can automatically fix this by running:');
console.error(` npx buildozer ${buildozerCmds.map(c => `'${c}'`).join(' ')} ${target}`);
console.error('Or to suppress this error, run:');
console.error(` npx buildozer 'set validate False' ${target}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: user may not have buildozer? also add simpler manual instructions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even worse: https://www.npmjs.com/package/buildozer
I think I'd better make this npx @bazel/buildozer
(note, that works even if you don't have it installed, so I think it's fine to keep the error message short since it should work in all cases)

@gregmagolan
Copy link
Collaborator

gregmagolan commented Mar 26, 2020

Looking this PR. another feature to add to the list to make it this feature compatible to ts_library is the expected_diagnostics attribute.
Just dropping this here as a mental note.

@alexeagle
Copy link
Collaborator Author

expected_diagnostics is annoying since you have to regex anyway. plus the kind of failure we want to test here is the action before tsc. Could we have a generic feature for run_node or npm_package_bin for expected failure?

@gregmagolan
Copy link
Collaborator

expected_diagnostics is annoying since you have to regex anyway. plus the kind of failure we want to test here is the action before tsc. Could we have a generic feature for run_node or npm_package_bin for expected failure?

Yes. Something with that could integration with run_node would be ideal. nodejs_test has expected_exit code but we have nothing for build rule failure testing

@alexeagle alexeagle merged commit 8644816 into bazel-contrib:master Mar 26, 2020
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.

3 participants