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

abstract error/checking parts of greta #529

Closed
njtierney opened this issue Jun 1, 2022 · 2 comments · Fixed by #687
Closed

abstract error/checking parts of greta #529

njtierney opened this issue Jun 1, 2022 · 2 comments · Fixed by #687
Labels
impact-1 High impact issue low hanging fruit Issue is simple to implement. Up Next
Milestone

Comments

@njtierney
Copy link
Collaborator

For example, I'm reasonably sure these lines

https://github.com/greta-dev/greta/blob/master/R/greta_model_class.R#L87-L172

Could be abstracted into some general checking function that checks whether nodes are not connected into each other. This would help make some of the code a bit tighter, but is a low priority.

@njtierney njtierney added this to the 0.6.0 milestone Jun 1, 2022
@njtierney
Copy link
Collaborator Author

I'm currently writing a lot of checking functions into descriptive checker functions that are in the checkers.R file. My process is that these can be refactored as time goes on, but the important thing is to make the function body of things in greta read more clearly. Sometimes the checking code can be complex and it interrupts understanding the intent of the function.

@njtierney
Copy link
Collaborator Author

A few more points about error checking code in greta

  1. They should use cli::cli_abort() instead of the current pattern of constructing a message then passing it to stop
  2. checking functions should have arg and call arguments, as discussed in the rlang docs
  3. "explainable variables" or short functions should be used to help simplify the if statements in error checking

I came across these as I was writing about the error checking code smell in a blog post, Code Smell: Error Handling Eclipse

@njtierney njtierney added Up Next low hanging fruit Issue is simple to implement. impact-1 High impact issue labels Jul 24, 2024
@njtierney njtierney modified the milestones: 0.6.0, 0.5.0 Jul 25, 2024
@njtierney njtierney modified the milestones: 0.5.0, 0.6.0 Jul 27, 2024
@njtierney njtierney moved this from Backlog to Weekly Todo in {greta} CRAN 0.5.0 release Aug 5, 2024
@njtierney njtierney moved this from Backlog to Weekly Todo in {greta} CRAN 0.5.0 release Aug 9, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in {greta} CRAN 0.5.0 release Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact-1 High impact issue low hanging fruit Issue is simple to implement. Up Next
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

1 participant