bingo
bingo copied to clipboard
jennybc
Help with maintenance
Hello @SamanthaToet @amysheep
I'm glad to hear you'd like to join in the fun here!
I propose this as a good way to start:
- You pick one (or many!) PRs and look them over and make a comment to the effect of LGTM ("looks good to me") or an indication of what might be problematic. I watch this repo, so I'll be notified of this.
Feel free to ask me questions. I assume this will evolve to giving you push permission and you could merge things yourselves.
There's probably other stuff missing here, such running the tests anywhere like travis. So there's also scope to make some improvements to bingo vs. just merging cards.
More ideas for possible contributions beyond approving cards:
Guidelines for submitting a new card. For example, I think there is some minimum number of lines that are necessary to work at all and to work "well". We need to spell that out somewhere, such as a CONTRIBUTING file. It could eventually also be something that is tested, so that incoming PRs with too few entries are flagged.
I haven't looked at this for a long time. I now wonder why the card data is stored as .R files, with c("a", "b") syntax, as opposed to to plain text files with one entry per line. Current Jenny thinks the latter seems much more sensible. Unless anyone can discover a reason for this peculiar past decision, it would be nice to switch over.
That's a great idea. I could try expanding on the CONTRIBUTING file so that's it's more of a "how to." Maybe we could also add something to the README? Down the line I think a test counting the number of lines would be really helpful. Actually, that doesn't need to be too far down the line... I can play around with that too.
That's a great idea. I could try expanding on the CONTRIBUTING file so that's it's more of a "how to."
Oh, there is already a CONTRIBUTING file! Yay. From my experience, people don't really read these and especially not if they are long. So this not a "more is more" situation. Your idea to mention the key fact about "# of squares" in the README is good. We should probably do that and call it a day.
I thought the number of squares per card is always 5 i.e. 5*5 per card. We can add test for the length of the topic list though. Maybe a minimum of 28?
@jennybc Are we requesting .R for submission or other formats are fine (e.g. .Rmd, .pdf...)?
Also, FREE is auto generated and not required to be in the c() list right? @jennybc
Are we requesting .R for submission
Yeah that's how this is currently setup.
Also, FREE is auto generated and not required to be in the c() list right?
I sure think so.
From double-checking some PRs, it's clear we need a test that would identify submissions that are just structurally wrong. I think almost 50% of the current batch would break the package.
Hi @pjmathews here's where we're getting thing going re: help with reviewing PRs (in the immediate sense, which is still much needed) and outlining some package enhancements that would make it much easier to check if PRs are structurally OK (for future and for building more package dev skills).
i'm going to make some tests for
- test topic .R file structure (is it a vector? is it a .R file)
- Capitalize the first letter?
- FREE shouldn't be in the list
The infrastructure is improving, which is going to make it much easier to accept PRs, from this crew re: the package and from others for bingo cards.
@amysheep added a test for the length of a topic's entries b62ed7ac6dc17d7726fed142f8aff8b934aeac8e
@AmeliaMN added .travis.yml 8d6a4ac283b5d380a3480e3a9c279d9aaa0f4e1d
I turned travis on, updated the config to include running test coverage, and turned on appveyor.
So now we can constantly see the status on the README and PRs will also be subjected to checks.
We still have a long way to go 😱 but this is a good start! This is a small package so getting this number up can go quickly.
I can look into why the card data is stored as .R files.
@amysheep I thought about this more and maybe it's to piggyback on R parsing the file for us, i.e. handling issues of embedded quotes or newlines or what have you. By forcing the user to write a valid c() call, we gain a lot, even though it looks a bit odd. If we just have a plaintext file that's supposed to have one square per line and we read it in, we are responsible for doing more validation.
@jennybc when some PRs fail the build, should we leave a message to the author and wait for them to correct the PRs or should we make changes directly on their PRs and merge in?
all_topics <- list.files(topics_dir, full.names = FALSE, pattern = "\\.R$")
The .onLoad function only pulls in the .R extension files so those submit with other format will pass the current tests because their files won't be load in to begin with.
We could change the onLoad function and remove the pattern argument, and add a test to check the .R format. What do you think @jennybc ?
when some PRs fail the build, should we leave a message to the author and wait for them to correct the PRs or should we make changes directly on their PRs and merge in?
I think we should comment and give the original poster a chance to fix the submission. After some suitable period of time, if we want the card, then I think it's appropriate to use, e.g., the usethis::pr_*() functions to finish the PR and, once it passes tests, merge.
The .onLoad function only pulls in the .R extension files so those submit with other format will pass the current tests because their files won't be load in to begin with.
It probably makes more sense to test that every file below inst/topics has certain qualities.