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

Initial infrastructure for the dual repo approach #25

Merged
merged 17 commits into from
Jun 6, 2024

Conversation

wlandau
Copy link
Member

@wlandau wlandau commented May 21, 2024

@shikokuchuo, this PR is a start on the infrastructure for the dual repo option. It modularizes and documents the different package checks in check_checks(), check_descriptions(), and check_versions(), and it runs those checks to record specific package issues via record_issues() the PR also adds a pkgdown website.

One minor snag is that the R-unvierse check API returns pretty JSON rather than newline-delimited json, so we can't use jsonlite::stream_in() to automatically paginate the results downloaded by check_checks(). If you or @jeroen have ideas, I would be curious to know. Manual pagination is inconvenient.

@wlandau wlandau requested a review from shikokuchuo May 21, 2024 20:41
@wlandau wlandau self-assigned this May 21, 2024
@wlandau
Copy link
Member Author

wlandau commented May 22, 2024

One minor snag is that the R-unvierse check API returns pretty JSON rather than newline-delimited json, so we can't use jsonlite::stream_in() to automatically paginate the results downloaded by check_checks(). If you or @jeroen have ideas, I would be curious to know. Manual pagination is inconvenient.

Thanks to @jeroen's tip about &stream=true, check_checks() now uses streaming paginated JSON via jsonlite::stream_in() (8ce5547).

@wlandau
Copy link
Member Author

wlandau commented May 22, 2024

In 1cad4f0, I added a new "date" field to each issue log. That way, we can see when a package first started having issues, and we can set a grace period for removal from the production universe. The date resets for a package when all the issues go away.

@wlandau
Copy link
Member Author

wlandau commented May 22, 2024

Two other things I could add, either in this PR or another one:

  1. Add a new check category for reverse dependencies. (check_dependencies() could flag an issue if there is an issue in a strong dependency. Would need to make sure this happens recursively over the whole DAG.)
  2. Generate the production packages.json manifest.

@wlandau
Copy link
Member Author

wlandau commented May 23, 2024

On second thought, (2) requires us to think through r-multiverse/help#57 a bit more.

@wlandau
Copy link
Member Author

wlandau commented May 24, 2024

In 50be3cd, I did a major refactor of this PR, including changing the names of many of the new functions.

As mentioned above, I plan to add an issues_dependencies() function (probably in a different PR) to flag packages whose revdeps have issues. But it's tough to do efficiently with just base R. I would prefer to use igraph to do the heavy lifting at scale, although this is a bit of a heavy dependency.

@shikokuchuo
Copy link
Member

It tests fine against the synthetic data, but I think it'd be easier to merge this and check it live.
Then new features can be added in smaller PRs :)

@wlandau
Copy link
Member Author

wlandau commented May 31, 2024

Thanks! Does that mean this is okay to merge?

I have been tied up with openpharma/brms.mmrm#107 this week, and I hope to spend more time on R-multiverse stuff next week.

@shikokuchuo
Copy link
Member

Yes, I just had one question about the need for utils_logic.R (see above). Thanks.

@wlandau
Copy link
Member Author

wlandau commented Jun 3, 2024

Yes, I just had one question about the need for utils_logic.R (see above). Thanks.

Sorry, I don't see your question in the comments or diff.

utils_logic.R is just for convenience, operators like %||% help make expressions compact. And I have seen them in packages I trust, e.g. https://github.com/r-lib/withr/blob/334415234c4b049d125c1a0b47a20d682daef9ad/R/utils.R#L97-L99. (Base R only just gained %||%, which checks in.null() rather than length(), which after reading HenrikBengtsson/Wishlist-for-R#120 (comment) I think may not be the most idiomatic choice.)

R/utils_logic.R Outdated Show resolved Hide resolved
@shikokuchuo
Copy link
Member

Sorry, I don't see your question in the comments or diff.

utils_logic.R is just for convenience, operators like %||% help make expressions compact. And I have seen them in packages I trust, e.g. https://github.com/r-lib/withr/blob/334415234c4b049d125c1a0b47a20d682daef9ad/R/utils.R#L97-L99. (Base R only just gained %||%, which checks in.null() rather than length(), which after reading HenrikBengtsson/Wishlist-for-R#120 (comment) I think may not be the most idiomatic choice.)

No worries, forgot to submit the review. My point is more for maintainability, as it may be neither of us modifying this code down the line. As base R has the null coalescing operator, it seems downright confusing to redefine this as something else and then have the 2 variants which are subtly different.

MM comments on this at RConsortium/S7#394 and RConsortium/S7#392 Again, I don't have a preference as to what is regarded as 'correct', but I think we can avoid any unnecessary pitfalls by only depending on one variant and not both.

@wlandau
Copy link
Member Author

wlandau commented Jun 6, 2024

@shikokuchuo, I finally had a free moment to address your comment on operators. Anything else?

@shikokuchuo
Copy link
Member

Please go ahead with the merge, thanks!

@wlandau wlandau merged commit dfc553a into r-multiverse:main Jun 6, 2024
6 checks passed
@wlandau wlandau deleted the dual-repo branch June 6, 2024 18:35
@wlandau
Copy link
Member Author

wlandau commented Jun 7, 2024

Just realized I forgot to make https://github.com/r-multiverse/multiverse.internals/releases/tag/0.2.0 the latest release yesterday. Just made the change now. Hopefully multiverse.internals 0.1.4 will become version 0.2.0 soon and I can migrate the CI/CD.

@wlandau
Copy link
Member Author

wlandau commented Jun 7, 2024

Looks to be working well, after some minor adjustments to the GitHub Actions workflows. https://github.com/r-multiverse/checks/tree/main/issues is a continually refreshing folder of JSON files. There is a JSON file for each package with issues, and each JSON file has specific details in machine-readable format. The next steps I am planning are:

  1. For each package with issues, flag all its recursive revdeps in https://github.com/r-multiverse/checks/tree/main/issues.
  2. Automatically build https://github.com/r-multiverse/production/blob/main/packages.json based on https://github.com/r-multiverse/multiverse/blob/main/packages.json and https://github.com/r-multiverse/checks/tree/main/issues.

I will submit a PR for each.

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.

2 participants