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

Add read_only flag to run_test #529

Merged
merged 3 commits into from
Jan 7, 2024
Merged

Add read_only flag to run_test #529

merged 3 commits into from
Jan 7, 2024

Conversation

TymekDev
Copy link
Contributor

@lorenzwalthert please let me know if that's what you had in mind in #511 (review)

@lorenzwalthert
Copy link
Owner

Thanks for that contribution, looks good 👍 . Now the last step would be to set the flag for each test, ideally automatically.

  • A natural place to add a read_only marker would be in the pre-commit-hooks.yaml but I don't know if extending this specs would then give a run-time warning or error when running the hooks.
  • Another option is to define the hook order content from the vignette in plain R code, e.g. a tibble, and then use R to read that table and pretty print it in the vignette.

Are you interested in investigating that further?

@TymekDev
Copy link
Contributor Author

I read your ideas, and initially was on board to implement this. However, starting with #510 some of the write-hooks might get --read-only flag. Once that's the case we would need a combination of hook name and parsing cmd_args to check for said flag.

With that in mind I think it would make sense to leave this up to the person implementing test and set this argument retrospectively according to the current state.

What do you think?

@lorenzwalthert
Copy link
Owner

Sorry for the delay. Yeah maybe we can leave that for now, but at least for the existing tests that we have, it would make sense to apply the flag, no? You added the functionality, but if I understand your PR correctly, it is never set?

@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Jan 6, 2024

Or did we agree to first implement the infra and then follow up? If yes, I think that's good for merging and we can follow up with #511 afterwards.

@lorenzwalthert
Copy link
Owner

Thanks again @TymekDev. 🥳

@lorenzwalthert lorenzwalthert merged commit 4f1a7e5 into lorenzwalthert:main Jan 7, 2024
17 checks passed
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