Re: [tor-bugs] #32522 [Core Tor/Tor]: Create better tooling for canonical tor header includes

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#32522: Create better tooling for canonical tor header includes
-------------------------------------------+-------------------------------
 Reporter:  teor                           |          Owner:  teor
     Type:  task                           |         Status:  needs_review
 Priority:  Medium                         |      Milestone:  Tor:
                                           |  0.4.3.x-final
Component:  Core Tor/Tor                   |        Version:
 Severity:  Normal                         |     Resolution:
 Keywords:  network-team-roadmap-november  |  Actual Points:  5
Parent ID:  #31851                         |         Points:  1
 Reviewer:  nickm                          |        Sponsor:  Sponsor31-can
-------------------------------------------+-------------------------------

Comment (by nickm):

 Hm. This looks like a complete rewrite to me.  That's okay, but I'm going
 to have to make a few general comments before I move in to a line-by-line
 review.  I hope that's okay.

 1) The changes made by this file look good, and the improved documentation
 and code structure is nice.

 2) I think this is the kind of rewrite where we want to have tests now.
 Do you think tests are in order here?

 3) Do you think that we should standardize on one of PRIVATE, INTERNAL, or
 EXPOSE?  Alternatively, do you think we should document the difference
 between them?

 4) Can/should we reuse python's logging framework rather than rolling our
 own set of warning/error reporting functions?

 5) When using "global", please make sure that you're actually modifying
 the variable.  It isn't necessary to say "global" when you're only reading
 the variable.

 6) This branch uses several different new maps, with new semantics.  Would
 it make sense to turn them into one or more classes? If not, we should at
 least have an overview listing all of them and what they're for.

 7) Does is really make sense to have "current_file" be a global?  Is there
 some more OO approach that would make the code cleaner?  (Obviously we
 shouldn't do that if it makes the code uglier.)

 7) Consider running this script through a python style checker, if you
 haven't done so already; it usually catches a few things when I remember
 to do that.

 8) I don't think that we should _remove_ any normalizations that this
 script does, but before we add any more normalizations, we should be sure
 that we aren't replicating work that our chosen code styling tool can
 already do for us.

 Once we've decided what to do with the above, I can start on a line-by-
 line review.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/32522#comment:4>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs