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

Silencing warnings in 4.6.2 Reviewers #806

Closed
wants to merge 2 commits into from
Closed

Silencing warnings in 4.6.2 Reviewers #806

wants to merge 2 commits into from

Conversation

thomaszwagerman
Copy link

@thomaszwagerman thomaszwagerman commented Feb 20, 2024

Hi there,

I was just having a read of the 4.6.2 Reviewers section and noticed the following warnings above the reviewer list output:

## Warning in value[[jj]][ri] <- if (is.factor(xij)) as.vector(xij) else xij:
## number of items to replace is not a multiple of replacement length

The reviewer author list is generated by the following code:

#| echo: false
#| results: 'asis'
#| eval: !expr has_airtable_access
editors <- c(
  "Noam Ross", "Karthik Ram", "Maëlle Salmon",
  "Anna Krystalli", "Mauro Lepore", 
  "Laura DeCicco", "Julia Gustavsen",
  "Emily Riederer", "Adam Sparks", "Jeff Hollister"
  )
reviewers <- airtabler::airtable(base = "app8dssb6a7PG6Vwj", 
                                table = "reviewers-prod")
reviewers <- reviewers$`reviewers-prod`$select_all()
reviewers <- reviewers[purrr::map_lgl(reviewers$reviews, 
                               ~!is.null(.)) & 
                         !(reviewers$name %in% c(editors, "???")), ]
# get last names
last_names <- humaniformat::last_name(trimws(reviewers$name))
reviewers <- reviewers[order(last_names), ]
reviewers$name[is.na(reviewers$name)] <- reviewers$github[is.na(reviewers$name)]
cat(paste0("[", reviewers$name, "](https://github.com/", reviewers$github, ")", collapse = " \U00B7 "))

I do not have access to the AIRTABLE_API_KEY (nor should I), so I cannot test further to suggest a tidier change to the code itself by running it. I guess there is a mismatch in the number of author names / GitHub links in the airtable?

I've added a #| warning: false output option to prevent it happening in the future, as I'm guessing that might be a common occurrence as author details change.

I am happy to contribute further changes if needed. I'm also happy to open an issue instead of this PR, to be addressed by an editor (with access to the API key) if that suits you better. Perhaps keeping warnings is desirable because they have flagged an error that may otherwise have gone unnoticed, but I will leave that up to you :)!

@mpadge
Copy link
Member

mpadge commented Feb 20, 2024

Thanks @thomaszwagerman. I approved the PR just to build and check. It builds at https://devdevguide.netlify.app/softwarereview_intro#editors-and-reviewers, where you can see for yourself that, unfortunately, the problem has not yet been solved. I'm confident @maelle will have further ideas here ...

@mpadge mpadge requested a review from maelle February 22, 2024 17:21
@maelle
Copy link
Member

maelle commented Feb 23, 2024

Thank you @thomaszwagerman!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants