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

Check that there are "enough" tests and documentation #492

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

MarkNahabedian
Copy link
Contributor

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.

check that a new package has enough test code and documentation.

Use a Dict to pass Guideline parameters from the arglist of run
opaquely through pull_request_build and check! to the various
guidelines.  Existing guideline parameters have not yet been moved to
this Dict.  Added this Dict parameter to every Guideline check
function.

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

Moved new code from previous change in src/utils.jl to
src/AutoMerge/util.jl.

Extracted depot setup code from AutoMerge.parse_registry_pkg_info to
new function setup_depot so that it is available to other guideline
tests.

"AutoMerge.linecounts_meet_thresholds" testset passes.
@MarkNahabedian
Copy link
Contributor Author

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

@ericphanson
Copy link
Member

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

@DilumAluthge
Copy link
Member

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?

Copy link
Member

@ericphanson ericphanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for your work on this! I will try to get PackageAnalyzer working on 1.3. In the meantime I've left some comments on other parts of the PR.

Project.toml Show resolved Hide resolved
src/AutoMerge/guidelines.jl Outdated Show resolved Hide resolved
src/AutoMerge/public.jl Show resolved Hide resolved
src/AutoMerge/types.jl Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
src/AutoMerge/guidelines.jl Outdated Show resolved Hide resolved
src/AutoMerge/util.jl Outdated Show resolved Hide resolved
MarkNahabedian and others added 8 commits January 25, 2023 19:57
…earlier. Make sure guideline_distance_check is last, as the comment says it should be.
…s in fun, as well as in the guideline function. Set the defaults to 0 so that once this code isa in place, the guideline is a no-op until configured in a workflow.
@GunnarFarneback
Copy link
Contributor

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.

Project.toml Outdated Show resolved Hide resolved
Failure messages with too much information.

Remove meets_threshold, which is no longer needed.
@MarkNahabedian
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Ping.

Is anyone going to review this?

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

@DilumAluthge DilumAluthge requested review from GunnarFarneback and ericphanson and removed request for GunnarFarneback and ericphanson April 12, 2023 23:55
@DilumAluthge
Copy link
Member

@ericphanson Would you be able to review this?

@DilumAluthge
Copy link
Member

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

@MarkNahabedian
Copy link
Contributor Author

@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.

Copy link
Member

@ericphanson ericphanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Otherwise, the implementation w/ separate parameters for counts & fractions looks good to me. Gives the most choice to the downstream registry.

@@ -102,7 +107,26 @@ function run(;
public_registries::Vector{<:AbstractString}=String[],
read_only::Bool=false,
environment_variables_to_pass::Vector{<:AbstractString}=String[],
# Four parameters for guideline_linecounts_meet_thresholds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Four parameters for guideline_linecounts_meet_thresholds
# Parameters for guideline_linecounts_meet_thresholds

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the count to "Seven".

I want to avoid confusion if additional parameters are added without a comment.

src/AutoMerge/guidelines.jl Outdated Show resolved Hide resolved
src/utils.jl Show resolved Hide resolved
test/automerge-unit.jl Outdated Show resolved Hide resolved
@DilumAluthge
Copy link
Member

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
Copy link
Contributor Author

MarkNahabedian commented Apr 18, 2023

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.

… documentation in their README file rather than using conventional Julia documentation.
@MarkNahabedian
Copy link
Contributor Author

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

I don't know how to proceed.

@MarkNahabedian
Copy link
Contributor Author

A small part of this PR is now broken out to #506.

Please review.

bors bot added a commit that referenced this pull request Apr 30, 2023
506: setup_depot for setting up a temporary depot for testing. r=ericphanson a=MarkNahabedian

I was asked to break up #492 into several parts.

This PR introduces the function `setup_depot` which pulls out the code for creating a temporary depot during testing so that it is easier to use in other places.


Co-authored-by: MarkNahabedian <[email protected]>
Co-authored-by: Mark Nahabedian <[email protected]>
@MarkNahabedian
Copy link
Contributor Author

Part of this change has been merged as #506

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.

Proposed automerge guideline: Require at least X lines of Julia code in /src, test, docs/readme/docstrings
4 participants