Opened 4 months ago

Closed 3 months ago

#30752 closed enhancement (fixed)

Practracker: usability improvements from May retrospective

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: network-team-roadmap-july
Cc: asn, nickm, teor Actual Points: .2
Parent ID: #29746 Points: 1.5
Reviewer: teor Sponsor: Sponsor31-can

Description

We talked about a few possible improvements to practracker in our May retrospective a couple of weeks back. Here are the suggestions we came up with.

  1. Practracker should have tolerances in its exceptions. For example, if an exception allows a function to be 150 lines long, then we should not cause an error until the function is (say) 155 lines long. If the function is 151-154 lines long, that should just be a warning.

These tolerances could be percentage-based, or just flat numbers.

There should be an option that makes all warnings fatal.

  1. There should be an option to list unused exceptions, and exceptions that are larger than they need to be.

Child Tickets

Change History (14)

comment:1 Changed 3 months ago by teor

Practracker failures are a bad and confusing experience for new contributors (and researchers).

Maybe it should be off by default? Or warn unless you set an env var that makes it fail?

comment:2 in reply to:  1 Changed 3 months ago by catalyst

Replying to teor:

Practracker failures are a bad and confusing experience for new contributors (and researchers).

I agree.

Maybe it should be off by default? Or warn unless you set an env var that makes it fail?

I think maybe the stricter options should only run on cron builds? As could the option that warns about exceptions that either could be lowered or deleted.

I think we might still want to have a (small?) tolerance that practracker will use when run from make check. But maybe we also want reviewers to look at small regressions in practices that are within the tolerances but that show up as warnings in the pull requests?

comment:3 Changed 3 months ago by nickm

Points: 1.5

comment:4 Changed 3 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:5 Changed 3 months ago by nickm

Actual Points: .2
Status: acceptedneeds_review

See my branch practracker_fixes with PR at https://github.com/torproject/tor/pull/1177 . It includes fixes for the issues above, and for the issues in #29746.

comment:6 Changed 3 months ago by gaba

Cc: nickm teor added
Keywords: network-team-roadmap-july added

comment:7 Changed 3 months ago by dgoulet

Reviewer: catalyst

comment:8 Changed 3 months ago by catalyst

Overall these look good. I'd like to do a bit of manual testing to make sure the changes work as advertised.

comment:9 Changed 3 months ago by catalyst

Minor comments:

typo in commit summary Pracktracker: give the number of new errors found.

The Makefile should provide a way to pass command line options in to practracker from the environment, so CI won't have to run practracker by bypassing the Makefile. This can be useful if we want to run with less-friendly options during cron builds, for example.

comment:10 Changed 3 months ago by catalyst

Reviewer: catalystteor

comment:11 Changed 3 months ago by nickm

The abovementioned branch now also contains fixes for #31202 (refactoring) and #31263 (integration tests).

Last edited 3 months ago by nickm (previous) (diff)

comment:12 Changed 3 months ago by nickm

Parent ID: #29746

comment:13 in reply to:  9 Changed 3 months ago by nickm

Replying to catalyst:

The Makefile should provide a way to pass command line options in to practracker from the environment, so CI won't have to run practracker by bypassing the Makefile. This can be useful if we want to run with less-friendly options during cron builds, for example.

Created #31309 for this issue.

comment:14 Changed 3 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged

Note: See TracTickets for help on using tickets.