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

MarkNahabedian · 2023-01-24T20:12:26Z

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.

…aned by PackageAnalyzer.

@warn

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 · 2023-01-25T04:07:31Z

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

ericphanson · 2023-01-25T23:14:41Z

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

DilumAluthge · 2023-01-25T23:27:51Z

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?

ericphanson

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

src/AutoMerge/guidelines.jl

src/AutoMerge/public.jl

src/AutoMerge/types.jl

src/utils.jl

src/AutoMerge/guidelines.jl

src/AutoMerge/util.jl

Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>

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

src/AutoMerge/guidelines.jl

GunnarFarneback · 2023-01-26T07:22:52Z

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

Failure messages with too much information. Remove meets_threshold, which is no longer needed.

…er than 1.6.

MarkNahabedian · 2023-02-03T17:57:08Z

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 · 2023-02-12T17:46:47Z

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 · 2023-04-12T23:44:57Z

Ping.

Is anyone going to review this?

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

DilumAluthge · 2023-04-12T23:55:58Z

@ericphanson Would you be able to review this?

DilumAluthge · 2023-04-12T23:56:17Z

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

MarkNahabedian · 2023-04-13T01:02:51Z

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

ericphanson

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.

ericphanson · 2023-04-16T13:13:01Z

src/AutoMerge/public.jl

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


Suggested change

# Four parameters for guideline_linecounts_meet_thresholds

# Parameters for guideline_linecounts_meet_thresholds

I'll update the count to "Seven".

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

src/AutoMerge/guidelines.jl

src/utils.jl

test/automerge-unit.jl

DilumAluthge · 2023-04-16T21:58:55Z

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.

…ideline_linecounts_meet_thresholds. Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>

Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>

MarkNahabedian · 2023-04-18T19:46:27Z

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 · 2023-04-24T16:39:44Z

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

I don't know how to proceed.

MarkNahabedian · 2023-04-28T18:06:54Z

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

Please review.

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 <naha@mit.edu> Co-authored-by: Mark Nahabedian <33787283+MarkNahabedian@users.noreply.github.com>

MarkNahabedian · 2023-05-01T01:27:25Z

Part of this change has been merged as #506

MarkNahabedian added 7 commits January 16, 2023 16:27

Add a registry Guideline to enforce thresholds against statistics gle…

fad9538

…aned by PackageAnalyzer.

Unit test for linecounts_meet_thresholds.

ec8f556

Record all Guidelines in ALL_GUIDELINES.

11026cd

Say which fields or keyargs are missing.

881dfba

Hopefully better warning message.

ebf74b8

Remove no longer applicable 'env_threshold' testset.

a6a07ac

MarkNahabedian mentioned this pull request Jan 24, 2023

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

Open

MarkNahabedian and others added 4 commits January 24, 2023 18:29

Merge branch 'master' into master

dd04259

Oops, I need to be more carefful commenting out loggins.

eee7c51

Merge branch 'master' of https://github.com/MarkNahabedian/RegistryCI.jl

d0b9996

PackageAnalyzer constrains Julia version to 1.6 or greater.

37bf9b6

ericphanson mentioned this pull request Jan 25, 2023

Branch to allow 1.3 to be installable JuliaEcosystem/PackageAnalyzer.jl#80

Closed

ericphanson linked an issue Jan 26, 2023 that may be closed by this pull request

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

Open

ericphanson reviewed Jan 26, 2023

View reviewed changes

MarkNahabedian and others added 8 commits January 25, 2023 19:57

Update src/AutoMerge/guidelines.jl

1b9e848

Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>

Update src/AutoMerge/guidelines.jl

75cbbf3

Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>

Revert 37bf9b6 since the problem it was intended to fix happens much …

48eb348

…earlier. Make sure guideline_distance_check is last, as the comment says it should be.

Merge branch 'master' of https://github.com/MarkNahabedian/RegistryCI.jl

cc90730

Compat entry for PackageAnalyzer.

6484879

Set defaults parameter values for guideline_linecounts_meet_threshold…

f36a8e0

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

Remove env_threshold_count which is no longer used.

4ed3e65

meets_threshold now asserts bounds on thresholds.

edcf26d

GunnarFarneback reviewed Jan 26, 2023

View reviewed changes

src/AutoMerge/guidelines.jl Show resolved Hide resolved

ericphanson reviewed Jan 26, 2023

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

ericphanson mentioned this pull request Jan 26, 2023

New version: PackageAnalyzer v0.2.0 JuliaRegistries/General#76413

Merged

MarkNahabedian added 2 commits February 1, 2023 20:05

Add a separate set of parameters for ratio thresholds.

d1e04e6

Failure messages with too much information. Remove meets_threshold, which is no longer needed.

Can't test guideline_linecounts_meet_thresholds on Julia versions old…

fc1d378

…er than 1.6.

DilumAluthge requested review from GunnarFarneback and ericphanson February 4, 2023 06:30

Update Project.toml

04ef6f3

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

Merge branch 'master' into master

8155a87

ericphanson reviewed Apr 16, 2023

View reviewed changes

MarkNahabedian and others added 5 commits April 18, 2023 15:20

Update src/AutoMerge/guidelines.jl documentation of parameters for gu…

009fae8

…ideline_linecounts_meet_thresholds. Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>

Update guideline parameter count.

c8ea113

remove using Logging.

5d45493

Fix typo.

5f2d565

Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>

Merge branch 'master' of https://github.com/MarkNahabedian/RegistryCI.jl

f14b2a6

Attempt to accomodate those projects which shoose to put all of their…

58eb4be

… documentation in their README file rather than using conventional Julia documentation.

MarkNahabedian mentioned this pull request Apr 28, 2023

setup_depot for setting up a temporary depot for testing. #506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MarkNahabedian commented Jan 24, 2023

MarkNahabedian commented Jan 25, 2023

ericphanson commented Jan 25, 2023

DilumAluthge commented Jan 25, 2023

ericphanson left a comment

GunnarFarneback commented Jan 26, 2023

MarkNahabedian commented Feb 3, 2023

MarkNahabedian commented Feb 12, 2023

MarkNahabedian commented Apr 12, 2023

DilumAluthge commented Apr 12, 2023

DilumAluthge commented Apr 12, 2023

MarkNahabedian commented Apr 13, 2023

ericphanson left a comment

ericphanson Apr 16, 2023

MarkNahabedian Apr 18, 2023

DilumAluthge commented Apr 16, 2023

MarkNahabedian commented Apr 18, 2023 •

edited

MarkNahabedian commented Apr 24, 2023

MarkNahabedian commented Apr 28, 2023

MarkNahabedian commented May 1, 2023

	# Four parameters for guideline_linecounts_meet_thresholds
	# Parameters for guideline_linecounts_meet_thresholds

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

Are you sure you want to change the base?

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

Conversation

MarkNahabedian commented Jan 24, 2023

MarkNahabedian commented Jan 25, 2023

ericphanson commented Jan 25, 2023

DilumAluthge commented Jan 25, 2023

ericphanson left a comment

Choose a reason for hiding this comment

GunnarFarneback commented Jan 26, 2023

MarkNahabedian commented Feb 3, 2023

MarkNahabedian commented Feb 12, 2023

MarkNahabedian commented Apr 12, 2023

DilumAluthge commented Apr 12, 2023

DilumAluthge commented Apr 12, 2023

MarkNahabedian commented Apr 13, 2023

ericphanson left a comment

Choose a reason for hiding this comment

ericphanson Apr 16, 2023

Choose a reason for hiding this comment

MarkNahabedian Apr 18, 2023

Choose a reason for hiding this comment

DilumAluthge commented Apr 16, 2023

MarkNahabedian commented Apr 18, 2023 • edited

MarkNahabedian commented Apr 24, 2023

MarkNahabedian commented Apr 28, 2023

MarkNahabedian commented May 1, 2023

MarkNahabedian commented Apr 18, 2023 •

edited