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

fix: Handle relative buildifier path without warning #387

Conversation

ryanwilsonperkin
Copy link
Contributor

@ryanwilsonperkin ryanwilsonperkin commented May 1, 2024

Fixes #386

Relative paths for buildifier are now supported as of v0.10.0, the warning should not be shown when the buildifier executable exists at the relative path specified.

BEGIN_COMMIT_OVERRIDE
fix(buildifier): Handle relative buildifier path without warning #387
END_COMMIT_OVERRIDE

Relative paths for buildifier are now supported as of v0.10.0, the
warning should not be shown when the buildifier executable exists at the
relative path specified.
Copy link

google-cla bot commented May 1, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@ryanwilsonperkin
Copy link
Contributor Author

(Haven't contributed previously or written VSCode extensions, would appreciate any feedback from the maintainers on how best to setup for testing this)

Copy link
Collaborator

@vogelsgesang vogelsgesang left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution and for the bug report!

A couple of stylistic issues, but on a high-level you found the right place to fix this 🙂

Regarding testing: We currently don't have a good testing strategy, yet. In #366, I am about to introduce the first unit tests, but we are still far away from a satisfactory testing solution, and I don't want to put it on you to build that infrastructure for the overall project. Hence, I would be fine merging this without test cases

src/buildifier/buildifier_availability.ts Outdated Show resolved Hide resolved
src/buildifier/buildifier_availability.ts Outdated Show resolved Hide resolved
src/buildifier/buildifier_availability.ts Outdated Show resolved Hide resolved
src/buildifier/buildifier_availability.ts Outdated Show resolved Hide resolved
@vogelsgesang vogelsgesang changed the title Handle relative buildifier path without warning fix: Handle relative buildifier path without warning May 2, 2024
@vogelsgesang
Copy link
Collaborator

I just saw that our CI doesn't like the code formatting...
You can run npm run format-fix to fix the formatting

@ryanwilsonperkin
Copy link
Contributor Author

Thanks for the feedback @vogelsgesang ! Pushed some fixes

@vogelsgesang
Copy link
Collaborator

Looks good to me, thanks again!

@vogelsgesang vogelsgesang merged commit 60051c5 into bazel-contrib:master May 2, 2024
2 checks passed
@vogelsgesang
Copy link
Collaborator

The next release to the Marketplace will probably still be a while. In the meantime, you can get a prebuilt pre-release version from the GitHub Action artifacts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relative paths for buildifier still trigger warning on startup
2 participants