Skip to content

yq: allow any filename in srcs#496

Merged
alexeagle merged 3 commits intobazel-contrib:mainfrom
pjjw:pjjw/yq-allow-any-filename
Aug 30, 2023
Merged

yq: allow any filename in srcs#496
alexeagle merged 3 commits intobazel-contrib:mainfrom
pjjw:pjjw/yq-allow-any-filename

Conversation

@pjjw
Copy link
Copy Markdown
Contributor

@pjjw pjjw commented Aug 15, 2023 •
edited
Loading

we have a number of files in our repository that are yaml, despite not having a .yaml suffix in their filename, and this filter prevents us from using the yq rule on them.

Type of change

  • New feature or functionality (change which adds functionality)

Test plan

  • Covered by existing test cases

@pjjw pjjw marked this pull request as ready for review August 15, 2023 17:52
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Aug 15, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@jbedard jbedard requested a review from kormide August 15, 2023 23:05
@kormide
Copy link
Copy Markdown
Collaborator

kormide commented Aug 15, 2023

I think that we automatically set some yq flags based on the file types seen in srcs. See https://github.com/aspect-build/bazel-lib/blob/main/lib/yq.bzl#L165.

It would be great if you could add some tests for different input types but without extensions to test that nothing broke there.

@pjjw pjjw force-pushed the pjjw/yq-allow-any-filename branch from 2e6a06c to 41cf704 Compare August 22, 2023 20:24
@pjjw
Copy link
Copy Markdown
Contributor Author

pjjw commented Aug 22, 2023

@kormide sure, added for a smattering of test cases for yaml and json with different-than-expected file extensions. seems to work fine, PTAL and see if there's some combination i missed here.

@kormide
Copy link
Copy Markdown
Collaborator

kormide commented Aug 22, 2023

@kormide sure, added for a smattering of test cases for yaml and json with different-than-expected file extensions. seems to work fine, PTAL and see if there's some combination i missed here.

Thanks. LGTM with a minor nit: can you remove the .whatever/.dealerschoice extensions since the name of the test targets imply that they are testing no extension on the input file?

@pjjw pjjw force-pushed the pjjw/yq-allow-any-filename branch from 41cf704 to ea6ce6f Compare August 22, 2023 22:13
@pjjw
Copy link
Copy Markdown
Contributor Author

pjjw commented Aug 22, 2023

how about i change the test names instead? really meant wrong or different- "no" in the sense of "no hints from the extension". naming!

@pjjw
Copy link
Copy Markdown
Contributor Author

pjjw commented Aug 30, 2023

thanks! meant to circle back and fix that up.

@alexeagle
Copy link
Copy Markdown
Collaborator

alexeagle commented Aug 30, 2023

Thanks for contributing @pjjw !!

@alexeagle alexeagle merged commit 3e9577a into bazel-contrib:main Aug 30, 2023
alexeagle pushed a commit that referenced this pull request May 26, 2025
@tetromino
Prepare release 1.6.0 (#496)
1d15ea8 
And take the opportunity to reformat docs using a more modern Stardoc release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kormide kormide kormide approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pjjw @CLAassistant @kormide @alexeagle