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

feat: Add diff_test_failure_message attribute to write_source_file* #1030

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jameskuszmaul-brt
Copy link

This makes it so that developers can add a custom error message to the diff test failure messages when using write_source_file and write_source_files.

This makes it so that developers can add a custom error message to the
diff test failure messages when using `write_source_file` and
`write_source_files`.
@jameskuszmaul-brt
Copy link
Author

Note: I would be happy to get either more automated tests added or change the name of the attribute; however, I didn't see any preexisting tests for these failure messages and so I didn't want to go around refactoring/messing with things without knowing where to look, and if you all don't consider it necessary to add automated tests for this then I didn't want to spend the time figuring it out.

@jameskuszmaul-brt jameskuszmaul-brt changed the title Add diff_test_failure_message attribute to write_source_file* feat: Add diff_test_failure_message attribute to write_source_file* Jan 15, 2025
@alexeagle alexeagle requested a review from kormide January 15, 2025 22:46
@kormide
Copy link
Collaborator

kormide commented Jan 17, 2025

IMO appending to the end seems like a very specific use case. A more generally useful solution would be to replace the whole message while supporting templating for the target and suggested target to be inserted.

@jameskuszmaul-brt
Copy link
Author

@kormide Do you think such templating should just go off of the target name or off of the whole pre-provided string?

Also, is there any standard pattern for what syntax you'd use for this sort of templating? I'd probably default to just implenting templating for the two target names and make it so that if you wanted to reproduce the existing error message you would do something like:

If you want to update all the files, do bazel run %all-targets%; if you want to update just this target, run %target%.

Suggestions on names & syntax welcome; I'm not inclined to implement any larger a feature set than described here.

@kormide
Copy link
Collaborator

kormide commented Jan 17, 2025

@kormide Do you think such templating should just go off of the target name or off of the whole pre-provided string?

Also, is there any standard pattern for what syntax you'd use for this sort of templating? I'd probably default to just implenting templating for the two target names and make it so that if you wanted to reproduce the existing error message you would do something like:

If you want to update all the files, do bazel run %all-targets%; if you want to update just this target, run %target%.

Suggestions on names & syntax welcome; I'm not inclined to implement any larger a feature set than described here.

I think the {{VAR_NAME}} syntax would be suitable since we use that elsewhere in expand_template. We could have three variables: TARGET, SUGGESTED_UPDATE_TARGET, and ORIGINAL_MESSAGE, the latter containing the full message. Then it would be trivial for you to append something after that.

@jameskuszmaul-brt
Copy link
Author

I think the {{VAR_NAME}} syntax would be suitable since we use that elsewhere in expand_template. We could have three variables: TARGET, SUGGESTED_UPDATE_TARGET, and ORIGINAL_MESSAGE, the latter containing the full message. Then it would be trivial for you to append something after that.

Sounds good; I'm not sure if I'll implement ORIGINAL_MESSAGE or not (depends on how much complexity it seems like it adds).

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.

2 participants