Opened 9 months ago

Closed 8 months ago

#31675 closed task (fixed)

Split microdescs_parse_from_string() into smaller functions

Reported by: nickm Owned by: nickm
Priority: Low Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 042-can asn-merge
Cc: Actual Points: .3
Parent ID: Points: 0
Reviewer: ahf Sponsor: Sponsor31-can


Instead of making an extended practracker exception here, we should make this function conform to our best practices.

I'm taking this on because I need a quick finger exercise between larger things.

Child Tickets

Change History (12)

comment:1 Changed 9 months ago by nickm

Branch is ticket31675; PR at . I'll put this in needs_review when CI passes.

For review, let's also think about the question: is this a good model for handling function-size practracker violations?

comment:2 Changed 9 months ago by nickm

CI passed, but I would like more test coverage.

comment:3 Changed 9 months ago by nickm

Status: assignedneeds_review

comment:4 Changed 9 months ago by nickm

(I've added tests, and gotten them passing)

comment:5 Changed 9 months ago by asn

Reviewer: asn

comment:6 Changed 9 months ago by ahf

Reviewer: asnahf

comment:7 Changed 9 months ago by nickm

Actual Points: .3
Keywords: 042-can added

comment:8 Changed 9 months ago by ahf

Status: needs_reviewneeds_revision

I get an error when logging into GH right now, so the review comes inline here:

I can see most of these things comes from code movement, but I think we can clean them up in follow up patches while this refactoring is happening:

On line 133 in microdesc_parse.c: There is a typo in the comment: witll should be will.
On line 143 in microdesc_parse.c: Minor detail, but copy_body could become a bool here.
On line 147 in microdesc_parse.c: no_onion_key can be promoted to a bool.
On line 319 in microdesc_parse.c: body_not_found can also be turned into a bool.

Otherwise I think the refactoring looks good.

comment:9 Changed 9 months ago by nickm

Status: needs_revisionneeds_review

Thanks for the review! I've added a fixup commit and a bool conversion commit.

comment:10 Changed 9 months ago by ahf

Status: needs_reviewmerge_ready

Looks good. Thanks!

comment:11 Changed 9 months ago by nickm

Keywords: asn-merge added

Thanks! I have squashed and re-pushed.

comment:12 Changed 8 months ago by asn

Resolution: fixed
Status: merge_readyclosed


Note: See TracTickets for help on using tickets.