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

Do not test the class name or attributes of asis_output() #78

Closed
wants to merge 1 commit into from
Closed

Do not test the class name or attributes of asis_output() #78

wants to merge 1 commit into from

Conversation

@yihui
Copy link
Contributor

@yihui yihui commented May 17, 2016

These are only implementation details of knitr, and can change without notice.

These are only implementation details of knitr, and can change without notice.
@krlmlr
Copy link
Member

@krlmlr krlmlr commented May 17, 2016

I'd like to verify that my code really returns a "knit_asis" object which is cacheable. The public knitr API doesn't currently seem to allow this. I'll use mocking to test this, then.

NB: I was unable to deduce the semantics of "cacheable = NA" from ?asis_output.

@krlmlr krlmlr closed this in 19235d2 May 17, 2016
@krlmlr
Copy link
Member

@krlmlr krlmlr commented May 17, 2016

Oh -- thanks anyway for the PR and the clarification.

@yihui yihui deleted the yihui:patch-1 branch May 17, 2016
@yihui
Copy link
Contributor Author

@yihui yihui commented May 18, 2016

The cacheable attribute was mainly designed for HTML widgets. The widget objects have HTML dependencies, so they could not be cached (otherwise the dependency information will be lost and the HTML document won't be rendered correctly). In the vast majority of cases, I don't think anybody need to care about cacheable at all. If all you care is the printed output of an object, this object is cacheable (in other words, printing does not have other significant side effects). I think tibble objects are always cacheable, so there is no need to test it. I agree tests are important, but it seems you are testing too much in this case...

@krlmlr
Copy link
Member

@krlmlr krlmlr commented May 18, 2016

I agree that tests should be designed in a way so that changes in dependencies do not break them. I've corrected the tests to make them more robust, so that a change in knitr internals shouldn't break them anymore.

krlmlr pushed a commit that referenced this pull request Jun 13, 2016
- Reworked output: More concise summary, removed empty line, showing number of hidden rows and columns (#51).
- Link to the package documentation from the `tibble` help page (#82).
- Don't rely on `knitr` internals for testing (#78).
krlmlr pushed a commit that referenced this pull request Jul 4, 2016
Follow-up release.

- `tibble()` is no longer an alias for `frame_data()` (#82).
- Remove `tbl_df()` (#57).
- `$` returns `NULL` if column not found, without partial matching. A warning is given (#109).
- `[[` returns `NULL` if column not found (#109).

- Reworked output: More concise summary (begins with hash `#` and contains more text (#95)), removed empty line, showing number of hidden rows and columns (#51). The trailing metadata also begins with hash `#` (#101). Presence of row names is indicated by a star in printed output (#72).
- Format `NA` values in character columns as `<NA>`, like `print.data.frame()` does (#69).
- The number of printed extra cols is now an option (#68, @lionel-).
- Computation of column width properly handles wide (e.g., Chinese) characters, tests still fail on Windows (#100).
- `glimpse()` shows nesting structure for lists and uses angle brackets for type (#98).
- Tibbles with `POSIXlt` columns can be printed now, the text `<POSIXlt>` is shown as placeholder to encourage usage of `POSIXct` (#86).
- `type_sum()` shows only topmost class for S3 objects.

- Strict checking of integer and logical column indexes. For integers, passing a non-integer index or an out-of-bounds index raises an error. For logicals, only vectors of length 1 or `ncol` are supported. Passing a matrix or an array now raises an error in any case (#83).
- Warn if setting non-`NULL` row names (#75).
- Consistently surround variable names with single quotes in error messages.
- Use "Unknown column 'x'" as error message if column not found, like base R (#94).
- `stop()` and `warning()` are now always called with `call. = FALSE`.

- The `.Dim` attribute is silently stripped from columns that are 1d matrices (#84).
- Converting a tibble without row names to a regular data frame does not add explicit row names.
- `as_tibble.data.frame()` preserves attributes, and uses `as_tibble.list()` to calling overriden methods which may lead to endless recursion.

- New `has_name() (#102).
- Prefer `tibble()` and `as_tibble()` over `data_frame()` and `as_data_frame()` in code and documentation (#82).
- New `is.tibble()` and `is_tibble()` (#79).
- New `enframe()` that converts vectors to two-column tibbles (#31, #74).
- `obj_sum()` and `type_sum()` show `"tibble"` instead of `"tbl_df"` for tibbles (#82).
- `as_tibble.data.frame()` gains `validate` argument (as in `as_tibble.list()`), if `TRUE` the input is validated.
- Implement `as_tibble.default()` (#71, tidyverse/dplyr#1752).
- `has_rownames()` supports arguments that are not data frames.

- Two-dimensional indexing with `[[` works (#58, #63).
- Subsetting with empty index (e.g., `x[]`) also removes row names.

- Document behavior of `as_tibble.tbl_df()` for subclasses (#60).
- Document and test that subsetting removes row names.

- Don't rely on `knitr` internals for testing (#78).
- Fix compatibility with `knitr` 1.13 (#76).
- Enhance `knit_print()` tests.
- Provide default implementation for `tbl_sum.tbl_sql()` and `tbl_sum.tbl_grouped_df()` to allow `dplyr` release before a `tibble` release.
- Explicit tests for `format_v()` (#98).
- Test output for `NULL` value of `tbl_sum()`.
- Test subsetting in all variants (#62).
- Add missing test from dplyr.
- Use new `expect_output_file()` from `testthat`.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants