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_revision
 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 teor):

 Replying to [comment:4 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?

 Yes, and the script can be re-targeted at a test directory using its
 command-line options.

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

 Yes, probably PRIVATE. I can make this change, and then modify the script.

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

 Probably, if we can work out how to get the current file context into it.

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

 I think this is tied up with 4) and 7).

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

 Probably one or two classes, implemented with an internal map.

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

 I'll see if python's logging has some kind of context argument.

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

 Do we have a recommended python style checker? Should we standardise on
 one, and start moving our scripts to it?

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

 I don't expect to add any more normalizations, my next step is #32655,
 which a styling tool definitely can't do.

 > 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:6>
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