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

Make message more informative #69

Merged
merged 11 commits into from
Oct 29, 2021
Merged

Make message more informative #69

merged 11 commits into from
Oct 29, 2021

Conversation

rossellhayes
Copy link
Contributor

@rossellhayes rossellhayes commented Oct 13, 2021

  • length problem message now includes the first missing or unexpected value if two conditions are met:
    • The difference between the expected and actual length ≤ 2.
    • There are ≤ 2 different values between the expected and actual vector.
    • In cases where the vector lengths differ by more than two or where there are more than two different values between the vectors, existing behavior is retained (i.e. only the difference in length is signalled).
library(tblcheck)

# Lengths differ <= 2 and <= 2 value differences
.result <- 1:3
.solution <- 1:5
vec_grade_length()
#> <gradethis_graded: [Incorrect]
#>   Your result should contain 5 values, but it has 3 values. I expected
#>   your result to include the value `4`.
#> >

# Lengths differ > 2
.result <- 1:3
.solution <- 1:6
vec_grade_length()
#> <gradethis_graded: [Incorrect]
#>   Your result should contain 6 values, but it has 3 values.
#> >

# > 2 value differences
.result <- 1:3
.solution <- 4:7
vec_grade_length()
#> <gradethis_graded: [Incorrect]
#>   Your result should contain 4 values, but it has 3 values.
#> >
  • values problem messages now inform the user of problems after the first max_diffs values:
library(tblcheck)

# Difference within first three values
.result <- 4:13
.solution <- 1:10
vec_grade_values()
#> <gradethis_graded: [Incorrect]
#>   The first 3 values of your result should be `1`, `2`, and `3`, not
#>   `4`, `5`, and `6`.
#> >

# Unexpected values after first three
.result <- c(1:3, 7:13)
.solution <- 1:10
vec_grade_values()
#> <gradethis_graded: [Incorrect]
#>   I didn't expect your result to include the values `11`, `12`, and
#>   `13`.
#> >

# Missing values after first three
.result <- c(1:3, rep(4L, 7))
.solution <- 1:10
vec_grade_values()
#> <gradethis_graded: [Incorrect]
#>   I expected your result to include the values `5`, `6`, and `7`.
#> >

Created on 2021-10-21 by the reprex package (v2.0.1)

Created on 2021-10-13 by the reprex package (v2.0.1)

Miscellaneous

  • Include vec_*_length() as alias for vec_*_dimensions()

For #65.

@rossellhayes rossellhayes added the enhancement New feature or request label Oct 13, 2021
@rossellhayes rossellhayes self-assigned this Oct 13, 2021
@rossellhayes rossellhayes marked this pull request as ready for review October 21, 2021 23:10
@gadenbuie
Copy link
Member

It feels like we could be doing a little more here and showing both values if there are 2 or fewer mistakes.

# Lengths differ <= 2 and <= 2 value differences
.result <- 1:3
.solution <- 1:5
vec_grade_length()
#> <gradethis_graded: [Incorrect]
#>   Your result should contain 5 values, but it has 3 values. I expected
#>   your result to include the values `4` and `5`.
#> >

I'm sure we could get into complicated territory around giving better descriptors, e.g.

I expected your result to include the values 4 and 5 after 3.

or showing up to n values when they are contiguous. But that's also a lot more complicated and we can wait on that.

In general, I think we should try hard to make clear the total number of problematic values, even if we only show 1-3 of them explicitly. In the following example, only showing the first 3 values without any more information makes it seem like there the result is off by three when it's in fact off by 6 values.

# Missing values after first three
.result <- c(1:3, rep(4L, 7))
.solution <- 1:10
vec_grade_values()
#> <gradethis_graded: [Incorrect]
#>   I expected your result to include the values `5`, `6`, and `7`.
#> >

We could instead add a little more context

I expected your result to include the values 5, 6, and 7 and 3 other values.

But more broadly, I think we might need to think about an overall different approach for some length/values problems that handles both at the same time to try to give more helpful feedback.

@gadenbuie gadenbuie merged commit 86f57a0 into main Oct 29, 2021
@gadenbuie gadenbuie deleted the improve-messages branch October 29, 2021 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants