-
Notifications
You must be signed in to change notification settings - Fork 11
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
84 expect wrapper #99
Conversation
resolves #86
* changes names to `old` and `new` to be more explicit * removing use of \" for escaping double-quotes and just wrapping in single quotes * some formatting
* remove showStyle field -- no need to have this * hey, make sure the style actually shows something * clean out some names
* add family tag * remove redundant tags * group tags better (like grouping)
maybe changes a bit too much (related to #91)
Yay, everything's green! is this ready for review? |
A rarity that something I did actually increased codecov! Yes, ready for review. |
Alright, I will begin with that Monday afternoon (traveling for the weekend). Guess all my nitpicking can be solved after merging, but I'm keen to understand your changes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. minor todos, only real question I have is if the wrapper test that uses waldo can live in tests instead as a regular function.
stop(msg, call. = FALSE) | ||
} | ||
|
||
|
||
which(replaceXMLEntities(wb$sheet_names) == sheetName) | ||
which(replaceXMLEntities(wb$sheet_names) == sheet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the wb_create_font_node() below was already removed in main iirc.
looks good to me! feel free to merge |
resolves #82
resolves #86