lockedata / lockeutils

Utils R package for the Locke Data team :art: :chart:
Other
1 stars 0 forks source link

have interactive prompt for indexing fonts #10

Closed DaveParr closed 5 years ago

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 28


Changes Missing Coverage Covered Lines Changed/Added Lines %
R/zzz.R 2 10 20.0%
<!-- Total: 2 10 20.0% -->
Files with Coverage Reduction New Missed Lines %
R/zzz.R 1 42.11%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 22: -5.0%
Covered Lines: 42
Relevant Lines: 73

💛 - Coveralls
maelle commented 6 years ago

Shouldn't this happen only when the font table is empty? I'm not sure.

maelle commented 6 years ago

I created a conflict, sorry.

Anyhow I think that on top of interactive this question should be asked when nrow(fnt) == 0?

maelle commented 6 years ago

Yay, thanks! Add yourself as "aut" in DESCRIPTION and then I'll merge.

To do that install the whoami package and the desc package and run desc::add_me(role = "aut").

DaveParr commented 6 years ago

Was just about to start writing a test for this for coveralls stopped being grumpy. Worth doing or shall I just mark it as #nocov?

maelle commented 6 years ago

ah if you have time it'd be grand! I'd suggest doing it with mocking! Ping me if you have never done that.

maelle commented 6 years ago

@DaveParr you included yourself twice! 👯‍♂️

maelle commented 6 years ago

sorry to be nitpicky, but the more I think of it, the more I'd prefer a mocking approach because it seems more logical not to add testing as a condition in the code?

with_mock(
    is_interactive = function() TRUE,
    count_fonts = function() 0,
    expect_message blabla
)

What I do not know is how to check the behavior of the package depending on what the user inputs. All of these are nice-to-have's anyway. :-)

maelle commented 6 years ago

Editing to add: testing the behavior to the user's response would be done by once again mocking something, in that case the menu function. But not to be added in your PR, it's all nice but not necessary!

maelle commented 5 years ago

@DaveParr do you think you can edit the test (to use mocking) soon? Then I'll merge (no need to test the behavior depending on the user's input)

maelle commented 5 years ago

does the build fail locally too?