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

Change check fail class and print method #114

Merged
merged 19 commits into from
Sep 5, 2024

Conversation

annakrystalli
Copy link
Member

check_failure elevated to error

To make clearer that all checks, including those resulting in a check_failure, are required to pass for files to be considered valid, check_failure class objects are elevated to errors (#111). Also, to make it easier for users to identify errors from visually scanning the printed output of hub_validation objects, the following custom bullets have been assigned to the print-out cli theme.

  • : check_failure class object.
  • : check_error class object. This also indicates early termination of the validation process.
  • : check_exec_error class object. This indicates an error in the execution of a check function.

Remove Octolog dependency

I also took the opportunity to remove the octolog dependency. This removes the annotation of validation results onto GitHub Action workflow logs (#113). Context:

I had been using octolog to wrap cli messages so that results of validations are returned on the GitHub Action page as annotations at the bottom of the workflow log.

It wasn't really working great, nor being used and octolog is still only available on GitHub with no known CRAN release date, I'm going to remove it as a dependency.

Everything prints much better on our documentation site now though:

THIS (where the check results are squased into a single line):
image

TO THIS (where test results and symbols are correctly printed on new lines)
image

Copy link

github-actions bot commented Sep 2, 2024

Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

I'm all for this! I made a couple of suggestions on order and wording, but they are non-blocking.

I'm especially in favor of removing the octolog dependency (though this might be my "not built here" disease flaring up). I think if we want to use it, it would be a good idea to create a fork, add it to our universe, and modify it to actually work for us (e.g. have it respect the IN_PKGDOWN envvar).

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
vignettes/articles/hub-validations-class.Rmd Outdated Show resolved Hide resolved
vignettes/articles/hub-validations-class.Rmd Outdated Show resolved Hide resolved
vignettes/articles/hub-validations-class.Rmd Outdated Show resolved Hide resolved
R/hub_validations_methods.R Outdated Show resolved Hide resolved
@zkamvar zkamvar linked an issue Sep 2, 2024 that may be closed by this pull request
@annakrystalli
Copy link
Member Author

Thanks for the review @zkamvar ! I've committed some suggestions or adapted them. Will wait for @bsweger's input too.

Copy link

@bsweger bsweger left a comment

Choose a reason for hiding this comment

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

This is fantastic--thanks, @annakrystalli!

Improve `hub_validations` print method
@annakrystalli annakrystalli merged commit 15adc77 into main Sep 5, 2024
8 checks passed
@annakrystalli annakrystalli deleted the ak/change-check-fail-class-and-print/111 branch September 5, 2024 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Remove octolog dependency
3 participants