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

enable auto-testing of problems #9

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@katrinleinweber
Collaborator

katrinleinweber commented Oct 3, 2017

As discussed in #5.

I'm seeing one or the other error, maybe related to file.path(get_exercism_path(), track_id, slug) introducing an extra /. But I presume that is because "[c]ache can probably be invalidated".

Katrin Leinweber added some commits Oct 3, 2017

@katrinleinweber

This comment has been minimized.

Show comment
Hide comment
@katrinleinweber

katrinleinweber Oct 3, 2017

Collaborator

About codecov/patch: Where do you recommend to place tests for this new functionality?

When I append the following to test-api.R

test_that("tests are run, and fail initially", {
  expect_output(fetch_problem(
      track_id = "r",
      slug = "hello-world",
      force = TRUE,
      open = FALSE,
      run_tests = TRUE
    ), "^DONE")
})

… and run testthat::auto_test_package(), the console just stalls at API methods: ..........123. Test-ception, apparently. With expect_error(), the output of the hello-world tests appears twice. It's like testthat is entering from the package-testing directly into the problem testing, but shouldn't it run both tests in a separate, temporary environment?

Collaborator

katrinleinweber commented Oct 3, 2017

About codecov/patch: Where do you recommend to place tests for this new functionality?

When I append the following to test-api.R

test_that("tests are run, and fail initially", {
  expect_output(fetch_problem(
      track_id = "r",
      slug = "hello-world",
      force = TRUE,
      open = FALSE,
      run_tests = TRUE
    ), "^DONE")
})

… and run testthat::auto_test_package(), the console just stalls at API methods: ..........123. Test-ception, apparently. With expect_error(), the output of the hello-world tests appears twice. It's like testthat is entering from the package-testing directly into the problem testing, but shouldn't it run both tests in a separate, temporary environment?

@jonmcalder

This comment has been minimized.

Show comment
Hide comment
@jonmcalder

jonmcalder Oct 3, 2017

Owner

Haven't got time to dig into the detail so I can't offer any suggestions regarding the errors and testing process but will review later this evening.

I think it's probably best to create a new test script which focuses on testing this functionality in isolation before doing integration tests with fetch_problem().

Owner

jonmcalder commented Oct 3, 2017

Haven't got time to dig into the detail so I can't offer any suggestions regarding the errors and testing process but will review later this evening.

I think it's probably best to create a new test script which focuses on testing this functionality in isolation before doing integration tests with fetch_problem().

@jonmcalder

jonmcalder requested changes Oct 3, 2017 edited

Could I ask that you rename run_tests() to ex_autotest() and rename the test argument in fetch_problem() to autotest please?

I'm planning to update all exported functions to begin with the prefix ex_ since I think this is useful in terms of UX. Will make it easier to find exercism functions with autocomplete when the package is loaded.

And I'd also like to distinguish this 'auto testing' with the usual manual testing. Especially since I'd also like to add a manual testing helper to the package (which can mitigate the need to switch working directories when sourcing the provided test scripts directly).

@katrinleinweber

This comment has been minimized.

Show comment
Hide comment
@katrinleinweber

katrinleinweber Oct 3, 2017

Collaborator

Is there some background reading you can recommend about this prefixing? Noticed it in stringi as well and am a bit skeptical TBH. I currently feel that it kinda pollutes or overloads the function name with a mnemonic, while saving only very few keystrokes: exe<KEY-UP><TAB> already narrows the autocomplete scope to the same effect.

IMHO, in an increasingly crowded namespace, prefixing one's own function names is overall less useful, than keeping one's own namespace clean and clear.

Collaborator

katrinleinweber commented Oct 3, 2017

Is there some background reading you can recommend about this prefixing? Noticed it in stringi as well and am a bit skeptical TBH. I currently feel that it kinda pollutes or overloads the function name with a mnemonic, while saving only very few keystrokes: exe<KEY-UP><TAB> already narrows the autocomplete scope to the same effect.

IMHO, in an increasingly crowded namespace, prefixing one's own function names is overall less useful, than keeping one's own namespace clean and clear.

@jonmcalder

This comment has been minimized.

Show comment
Hide comment
@jonmcalder

jonmcalder Oct 3, 2017

Owner

The catalyst for me in this case was an issue @jennybc raised here that I stumbled upon earlier this year and reminded me of the idea (if she spots this maybe she'll be kind enough to chime in here?).

I can't recommend any background reading because I can't recall coming across any substantial motivations for it, I can only share my current thinking:

  • Firstly, I think that using common prefixes for function names is probably more about aiding discoverability than it is about reducing keystrokes. This is especially true for newer useRs and/or interactive usage as opposed to development usage where namespacing is expected. With that said,
    in this case I think it would help in both respects.

  • Given exercism.io's target market, I'd say this package is largely aimed at people who are newer to R, who (I would assume) are less likely to be familiar with namespacing:: a function call (I certainly wasn't when I started out), unless they are developers coming from another language.

  • Users (who don't know about namespacing or can't remember the name of the package - though the latter is less likely in this case given the specific use case) are likely to begin searching for a function by typing the beginnings of or part of whatever they can remember of the name for the function they want and then inspecting auto completion options.

  • This is predominantly an API package, which means that many of the things the functions do are common (or common sounding) actions i.e. set, fetch, submit, skip, check etc, just with a specific context. This makes them simple and generic but it doesn't make them memorable. In fact in increases the chance of (partial) overlap with existing functions (e.g. the prefixes set..., skip..., check... are present in base, utils, testthat and more)

  • When you use stringr, stringi, forcats, googlesheets etc it's quickly apparent that many (if not most) of the function names in each case start with str_, stri_, fct_, gs_ etc. So those prefixes become familiar/memorable early on, and you don't need to know about namespacing in order to capitalize on the value of autocompletion to help you discover what you are looking for.

  • In the case of stringr and str_, it's additionally valuable that the function prefix is the same as the start of the package name, since the double match brings those functions to the top of the autocompletion list even sooner. The same would be true of exercism with ex_.

Could you elaborate/enlighten me in terms of what you see as potential negatives? Is it just that you feel it's unnecessary?

Owner

jonmcalder commented Oct 3, 2017

The catalyst for me in this case was an issue @jennybc raised here that I stumbled upon earlier this year and reminded me of the idea (if she spots this maybe she'll be kind enough to chime in here?).

I can't recommend any background reading because I can't recall coming across any substantial motivations for it, I can only share my current thinking:

  • Firstly, I think that using common prefixes for function names is probably more about aiding discoverability than it is about reducing keystrokes. This is especially true for newer useRs and/or interactive usage as opposed to development usage where namespacing is expected. With that said,
    in this case I think it would help in both respects.

  • Given exercism.io's target market, I'd say this package is largely aimed at people who are newer to R, who (I would assume) are less likely to be familiar with namespacing:: a function call (I certainly wasn't when I started out), unless they are developers coming from another language.

  • Users (who don't know about namespacing or can't remember the name of the package - though the latter is less likely in this case given the specific use case) are likely to begin searching for a function by typing the beginnings of or part of whatever they can remember of the name for the function they want and then inspecting auto completion options.

  • This is predominantly an API package, which means that many of the things the functions do are common (or common sounding) actions i.e. set, fetch, submit, skip, check etc, just with a specific context. This makes them simple and generic but it doesn't make them memorable. In fact in increases the chance of (partial) overlap with existing functions (e.g. the prefixes set..., skip..., check... are present in base, utils, testthat and more)

  • When you use stringr, stringi, forcats, googlesheets etc it's quickly apparent that many (if not most) of the function names in each case start with str_, stri_, fct_, gs_ etc. So those prefixes become familiar/memorable early on, and you don't need to know about namespacing in order to capitalize on the value of autocompletion to help you discover what you are looking for.

  • In the case of stringr and str_, it's additionally valuable that the function prefix is the same as the start of the package name, since the double match brings those functions to the top of the autocompletion list even sooner. The same would be true of exercism with ex_.

Could you elaborate/enlighten me in terms of what you see as potential negatives? Is it just that you feel it's unnecessary?

@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc Oct 4, 2017

Yes I think the reason to consider a common prefix is that auto-complete makes it so easy to get a relevant universe of functions in front of one's eyeballs.

jennybc commented Oct 4, 2017

Yes I think the reason to consider a common prefix is that auto-complete makes it so easy to get a relevant universe of functions in front of one's eyeballs.

@jonmcalder

I did look into the issue of testing the testing and it's not trivial. Given that I'm ok with foregoing testing of this for now. I did some checks locally and it seems fine.

I don't know where the earlier errors you referenced are coming from though. Did you check that your exercism_path variable hasn't been reset or overwritten in the process of running the other utils/API tests? I think I still have work to do there to clean that up further...

Show outdated Hide outdated R/run_tests.R Outdated
@katrinleinweber

This comment has been minimized.

Show comment
Hide comment
@katrinleinweber

katrinleinweber Oct 7, 2017

Collaborator

Could you elaborate/enlighten me in terms of what you see as potential negatives? Is it just that you feel it's unnecessary?

No, it's not just that, see the IMHO above. I'm also deriving this stance from this warning against producing "simple, expedient, disposable code that adequately addresses just the problem at-hand.".

[…] double match brings those functions to the top of the autocompletion list even sooner. The same would be true of exercism with ex_.

True, and I'm not disputing the benefits. Just that they are not worth taking a step in an overall wrong direction. Wrong for the whole R ecosystem.

[…] you don't need to know about namespacing in order to capitalize on the value of autocompletion to help you discover what you are looking for.

I think knowing about namespacing (and using it in combination with auto-completion) is synergistically useful in the long run. And thus worth teaching / demanding, instead of avoiding.

[…] it's quickly apparent that many (if not most) of the function names in each case start with str_, stri_, fct_, gs_ etc.

I hope it's all of them ;-D Otherwise, inconsistent prefixing would "tear down with the ass, what you've (tried to) build with the hands".

So those prefixes become familiar/memorable early on […]

So would repeatedly used package and function names ;-)

Users […] are likely to begin searching for a function by typing the beginnings of or part of whatever they can remember of the name for the function they want and then inspecting auto completion options.

Yes, maybe they are, but is it helpful in the long run to train people to rely on the auto-complete pop-up rather than the docu / help pages? I think it's overall better to incentivize the latter. If that needs optimising, it's a task for the IDE developers.

[…] many of the things the functions do are common (or common sounding) actions i.e. set, fetch, submit, skip, check etc, just with a specific context. This makes them simple and generic but it doesn't make them memorable.

I don't think that should be our goal. Optimising for the mechanisms at hand (function list in the help pages, pkg & function name completion, auto-completion pop-up in RStudio) should be. The function names are (and should continue to be) clear about what they do. Mixing in a mnemonic prefix detracts from that.

Effectively, I'm not going to block this quick, expedient change (already committed it locally, actually), but these are my arguments against actually doing it. Essentially: it would be training wheels that are only mildly useful and eventually problematic.

[…] In fact i[t] increases the chance of (partial) overlap with existing functions (e.g. the prefixes set..., skip..., check... are present in base, utils, testthat and more)

Maybe R needs a mechanism to import specific_function from large_package to avoid overlap/conflicts?

Collaborator

katrinleinweber commented Oct 7, 2017

Could you elaborate/enlighten me in terms of what you see as potential negatives? Is it just that you feel it's unnecessary?

No, it's not just that, see the IMHO above. I'm also deriving this stance from this warning against producing "simple, expedient, disposable code that adequately addresses just the problem at-hand.".

[…] double match brings those functions to the top of the autocompletion list even sooner. The same would be true of exercism with ex_.

True, and I'm not disputing the benefits. Just that they are not worth taking a step in an overall wrong direction. Wrong for the whole R ecosystem.

[…] you don't need to know about namespacing in order to capitalize on the value of autocompletion to help you discover what you are looking for.

I think knowing about namespacing (and using it in combination with auto-completion) is synergistically useful in the long run. And thus worth teaching / demanding, instead of avoiding.

[…] it's quickly apparent that many (if not most) of the function names in each case start with str_, stri_, fct_, gs_ etc.

I hope it's all of them ;-D Otherwise, inconsistent prefixing would "tear down with the ass, what you've (tried to) build with the hands".

So those prefixes become familiar/memorable early on […]

So would repeatedly used package and function names ;-)

Users […] are likely to begin searching for a function by typing the beginnings of or part of whatever they can remember of the name for the function they want and then inspecting auto completion options.

Yes, maybe they are, but is it helpful in the long run to train people to rely on the auto-complete pop-up rather than the docu / help pages? I think it's overall better to incentivize the latter. If that needs optimising, it's a task for the IDE developers.

[…] many of the things the functions do are common (or common sounding) actions i.e. set, fetch, submit, skip, check etc, just with a specific context. This makes them simple and generic but it doesn't make them memorable.

I don't think that should be our goal. Optimising for the mechanisms at hand (function list in the help pages, pkg & function name completion, auto-completion pop-up in RStudio) should be. The function names are (and should continue to be) clear about what they do. Mixing in a mnemonic prefix detracts from that.

Effectively, I'm not going to block this quick, expedient change (already committed it locally, actually), but these are my arguments against actually doing it. Essentially: it would be training wheels that are only mildly useful and eventually problematic.

[…] In fact i[t] increases the chance of (partial) overlap with existing functions (e.g. the prefixes set..., skip..., check... are present in base, utils, testthat and more)

Maybe R needs a mechanism to import specific_function from large_package to avoid overlap/conflicts?

@katrinleinweber

This comment has been minimized.

Show comment
Hide comment
@katrinleinweber

katrinleinweber Oct 7, 2017

Collaborator

[…] consider a common prefix [so] that auto-complete makes it so easy to get a relevant universe of functions in front of one's eyeballs.

pkg_name:: achieves the same already, can be auto-completed just as well, and IMHO trains package users to write more explicit code.

Dear @jennybc, I respect your work greatly, learned a lot from your teaching material and googlesheets helped me a lot. But function name prefixing, I have to counter, is an example of Einstein's (?) "But Not Simpler" aphorism.

Collaborator

katrinleinweber commented Oct 7, 2017

[…] consider a common prefix [so] that auto-complete makes it so easy to get a relevant universe of functions in front of one's eyeballs.

pkg_name:: achieves the same already, can be auto-completed just as well, and IMHO trains package users to write more explicit code.

Dear @jennybc, I respect your work greatly, learned a lot from your teaching material and googlesheets helped me a lot. But function name prefixing, I have to counter, is an example of Einstein's (?) "But Not Simpler" aphorism.

Katrin Leinweber added some commits Oct 7, 2017

Katrin Leinweber
Katrin Leinweber
@lorenzwalthert

This comment has been minimized.

Show comment
Hide comment
@lorenzwalthert

lorenzwalthert Oct 7, 2017

Asked to comment by @jonmcalder. I agree with @katrinleinweber and pretty much all the arguments brought up above (which I don't need to repeat explicitly). One of the things that should be avoided pretty much at any cost in programming IMO is redundancy and duplication. I think this is promoted with a style like

pkg_a::pkg_a_fun_a()

I know this is an extreme example, the overlap is not 100% usually. Maybe we should make a comparison with Python, where a variety of import options are available.

import pkg_a 
import pkg_a as a
from pkg_a import fun_a

R has (currently) only two for exported functions

library("pkga")
fun_from_a()
# or
pkga::fun_from_a()

So we can't do something like

alias_namespace("numpy", as = "np")
np::fun_from_np()

I think that would be neat. Anyone eager to implement that (if possible at all)?

In addition, if one wants to adhere to the tidyverse style guide:

  • function calls should be verbs. I don't think the proposed prefixes help in that regard - rather it's the contrary.
  • There are only two packages to my knowledge that implement such a style (well kind of): forcats and stringr so it seems not be a very dominant paradigm.

As far as new users go, I think

  • good documentation using roxygen r #' @family and r #' @seealso helps user to find related functions.
  • promoting of pkg::fun() is a fine to avoid name space conflicts but maybe a bit overcautious for most cases. When writing packages, one can be conservative and use #' @importFrom pkg fun to only import required functions.

So @jonmcalder as far as your renaming goes, I think it may be desirable for the function names to be a bit more verbose. So instead of

fetch()

etc. why not using

fetch_exercise()

?

I know that might be understood as contradicting to what I said above, but I think in your case it is fine if you have a suffix that contains a fraction of the package name because it has something to do with what the command does - and not primary with the name of the package. In particular, the command preserves a verb-like character.

lorenzwalthert commented Oct 7, 2017

Asked to comment by @jonmcalder. I agree with @katrinleinweber and pretty much all the arguments brought up above (which I don't need to repeat explicitly). One of the things that should be avoided pretty much at any cost in programming IMO is redundancy and duplication. I think this is promoted with a style like

pkg_a::pkg_a_fun_a()

I know this is an extreme example, the overlap is not 100% usually. Maybe we should make a comparison with Python, where a variety of import options are available.

import pkg_a 
import pkg_a as a
from pkg_a import fun_a

R has (currently) only two for exported functions

library("pkga")
fun_from_a()
# or
pkga::fun_from_a()

So we can't do something like

alias_namespace("numpy", as = "np")
np::fun_from_np()

I think that would be neat. Anyone eager to implement that (if possible at all)?

In addition, if one wants to adhere to the tidyverse style guide:

  • function calls should be verbs. I don't think the proposed prefixes help in that regard - rather it's the contrary.
  • There are only two packages to my knowledge that implement such a style (well kind of): forcats and stringr so it seems not be a very dominant paradigm.

As far as new users go, I think

  • good documentation using roxygen r #' @family and r #' @seealso helps user to find related functions.
  • promoting of pkg::fun() is a fine to avoid name space conflicts but maybe a bit overcautious for most cases. When writing packages, one can be conservative and use #' @importFrom pkg fun to only import required functions.

So @jonmcalder as far as your renaming goes, I think it may be desirable for the function names to be a bit more verbose. So instead of

fetch()

etc. why not using

fetch_exercise()

?

I know that might be understood as contradicting to what I said above, but I think in your case it is fine if you have a suffix that contains a fraction of the package name because it has something to do with what the command does - and not primary with the name of the package. In particular, the command preserves a verb-like character.

@lorenzwalthert

This comment has been minimized.

Show comment
Hide comment
@lorenzwalthert

lorenzwalthert Oct 7, 2017

I think it depends also a bit on how the package is used. E.g. with devtools, I noted that often, people do not load the package and always use devtools::fun().
As far as the function auto_test() goes, I would use it as it is used in test_that, i.e. not autotest() but auto_test() to be consistent.

lorenzwalthert commented Oct 7, 2017

I think it depends also a bit on how the package is used. E.g. with devtools, I noted that often, people do not load the package and always use devtools::fun().
As far as the function auto_test() goes, I would use it as it is used in test_that, i.e. not autotest() but auto_test() to be consistent.

@katrinleinweber

This comment has been minimized.

Show comment
Hide comment
@katrinleinweber

katrinleinweber Oct 8, 2017

Collaborator

I have no feelings (or maybe slightly positive ones) about …_exercise or …_solution or similar suffixes to function names, because as long as the first few characters are unique, auto-complete kicks in nicely.

About the run_test() vs. auto_test() vs. autotest(): How about start_testing()? In RStudio, there is a 🛑 button to stop testthat::auto…() again, so something with start would be a logical counterpart. Assuming that most of our users will use RStudio. Also, I would actually avoid reusing the same name, and we also have to think about keeping the same name logical as a parameter in the fetch functions.

Collaborator

katrinleinweber commented Oct 8, 2017

I have no feelings (or maybe slightly positive ones) about …_exercise or …_solution or similar suffixes to function names, because as long as the first few characters are unique, auto-complete kicks in nicely.

About the run_test() vs. auto_test() vs. autotest(): How about start_testing()? In RStudio, there is a 🛑 button to stop testthat::auto…() again, so something with start would be a logical counterpart. Assuming that most of our users will use RStudio. Also, I would actually avoid reusing the same name, and we also have to think about keeping the same name logical as a parameter in the fetch functions.

@katrinleinweber katrinleinweber deleted the 5-run-tests branch Oct 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment