Check that there are "enough" tests and documentation

[MarkNahabedian]

These changes introduce the new Guideline guideline_linecounts_meet_thresholds that uses PackageAnalyzer to collect line counts from the candidate package and compare against specified thresholds. The thresholds are provided ad these keyword arguments to AutoMerge.run:

• src_min_lines: Minimum number of lines of source code
• readme_min_lines: % Minimum number of lines in the README file
• test_min_lines: % Minimum number of lines of code in the test directory
• doc_min_lines: # Minimum number of lines of documentation

Those marked with a % can be expressed as a count, or as a fraction of the number of lines of source code. For test_min_lines and doc_min_lines, the denominator of the fraction also includes the number of lines of test code.

Additional work:

All Guidelines are collected into ALL_GUIDELINES to make them easier to identify and count.

run creates a Dict with which to opaquely pass parameters for the various Guidelines down through pull_request_build and check!. That Dict parameter has beed added to all Guideline check functions, most of which ignore it for now.

Report which keyword arguments are missing from GitHubAutoMergeData using @warn.

The depot setup code from the AutoMerge.parse_registry_pkg_info testset has been extracted to the new function setup_depot so that it can be used in other tests.

[MarkNahabedian]

How should I deal with the PackageAnalyzer version issue? PackageAnalyzer has a constraint that Julia version be 1.6 or greater.

[ericphanson]

Could we drop support for pre-Julia 1.6 in RegistryCI @DilumAluthge?

[DilumAluthge]

I really would prefer that we keep support for all Julia versions >= Julia 1.3. That way, we can add new registry consistency checks in the future, and those checks will be run on Julia 1.3.

PackageAnalyzer is only needed for AutoMerge, right? Can you register an empty version of PackageAnalyzer that is compatible with julia = "1"? It would contain no source code.

Alternatively, can we make PackageAnalyzer a weak dep for Julia 1.9+? Then, it will be ignored for Julia versions prior to 1.9, right?

[GunnarFarneback]

I also think we shouldn't require docs & readme, since some packages document their code via the readme. I think we should ask for 10-20 lines of docs OR readme

Agreed, it's rather pointless to require manual intervention for packages with README-only documentation or a very thin README pointing to the docs.

[MarkNahabedian]

Concerning requireing both docs and readme, I'm not able to respond in-line.

I agree that Either is sufficient and they're not both necessary. Docs isn't really sufficiholds.ent unless there's a link from the readme. I don't think PackageAnalyzer checks for that, and I'm willing to let the doc link slide for now.

Does someone want to propose logic for the docs/readme check?

Right now they must both exceed their respective thresholds?

Should it be sufficient that either meets its threshold?

I was originally thinking that the readme threshold would be a lower bar -- readme being a brief explanation while documentation provides full details about how to use the package. I feel that one is being inconsiderate of ones users if they don't provide good documentation.

Maybe if the docs threshold isn't met then the readme linecount should meet the docs threshold instead of the readme threshold?

2023-04-19: the documentation thresholds (count and fraction) now test the max of doc and README lines, instead of just doc lines. This is to accommodate projects for which all documentation is in the README.

[MarkNahabedian]

While we're critiqueing thresholds, I'll not that lines of test code is a lame surrogate for test coverage. Maybe that will be the topic of a future guideline, whence we can change the tests threshold for this guideline to 0.

[DilumAluthge]

I'd like to do refactoring in separate PRs from feature additions.

[MarkNahabedian]

https://github.com/MarkNahabedian/AnalyzeRegistryPackages.jl provides an analysis of the packages in the general registry.

The scatter plots should probably have excluded jlls, but they don't yet.

[MarkNahabedian]

I don't think there is anything more I can do here until there's a discussion about how to deasl with the docs v. README dichotomy mentioned above.

Comments?

[MarkNahabedian]

After a lot of muddle headed thinking, I have some plots that might help to inform what values we might choose for the various thresholds. The code to collect the data and the plots are at https://github.com/MarkNahabedian/AnalyzeRegistryPackages.jl.

There's also a TSV file of registered repositories that were unreachable in two attempts.

[MarkNahabedian]

Ping.

Is anyone going to review this?

If there is something more that I should do, please inform me.

[DilumAluthge]

@ericphanson Would you be able to review this?

[DilumAluthge]

@MarkNahabedian Can you rebase on the latest master, and fix the merge conflicts?

[MarkNahabedian]

@DilumAluthge:. I've never used rebase before, and from what I've read, I'm not comfortable with it.

I saw the merge conflict and managed to resolve some of it. I don't see how to resolve the one remaining diff though.

[DilumAluthge]

I think https://github.com/JuliaRegistries/RegistryCI.jl/pull/492/files#r1087494519 should still be addressed. If we want to rename the struct in a separate PR that seems fine, but this PR (adding a specific guideline) should not also be restructuring the data flow through all the guidelines!

I agree. In general, refactor/restructure PRs should be separate from "add new guideline" PRs.

@MarkNahabedian If you like, you can first make a PR to do the desired refactoring, and then once that PR has been reviewed and merged, you can rebase this PR on master. So that'll let you use the benefits of the refactoring in this PR.

[MarkNahabedian]

I think https://github.com/JuliaRegistries/RegistryCI.jl/pull/492/files#r1087494519 should still be addressed. If we want to rename the struct in a separate PR that seems fine, but this PR (adding a specific guideline) should not also be restructuring the data flow through all the guidelines!

I agree. In general, refactor/restructure PRs should be separate from "add new guideline" PRs.

@MarkNahabedian If you like, you can first make a PR to do the desired refactoring, and then once that PR has been reviewed and merged, you can rebase this PR on master. So that'll let you use the benefits of the refactoring in this PR.

I don't believe I've done any refactoring here. I have proposed refactoring in an issue cited above.

The codebase shows that how parameters are passed from the entry point to a guideline is arbitrary. Yes, I added a third way for parameters to be passed, but at some point in the past someone added a second.

I've spent more time on this change than I expected too. Removing passing a parameter dict feels like a step backwards. At the risk of seeming uncooperative, I don't want to spend any of my time on it. Any of the package maintainers can do so if they actually think this is important.

4/20: Sorry. It's been a while since I did this and forgot how much of this PR was due to passing the additional parameters. I can do a separate PR for that and another for setup_depot.

[MarkNahabedian]

I tried createing another fork for the guideline parameter change, but GitHub wouldn't let me.

I don't know how to proceed.

[MarkNahabedian]

A small part of this PR is now broken out to https://github.com/JuliaRegistries/RegistryCI.jl/pull/506.

Please review.

[MarkNahabedian]

Part of this change has been merged as https://github.com/JuliaRegistries/RegistryCI.jl/pull/506