Current coverage is 90.76%

Merging #1410 into master will increase coverage by 0.01%

@@             master      #1410   diff @@
==========================================
  Files            57         57          
  Lines          7264       7268     +4   
  Methods           0          0          
  Messages          0          0          
  Branches        434        434          
==========================================
+ Hits           6592       6597     +5   
+ Misses          606        605     -1   
  Partials         66         66          

Powered by Codecov. Last updated by 838a0c9...08348b5

Two thoughts --

• what about a small reusable inline-able class? There are a bunch of places to put this, and if we ever want to change up the implementation it might be handy to have it all nicely encapsulated.
• for this purpose, a threshold model might be better than using a modulus and comparing to 0. In Python land (for this and other projects), it's been handy to set a threshold, then print out progress as soon as that threshold is passed (and then increment the threshold). Also, moduluses are slower than comparisons :), although that's a minor issue.

I think a small class sitting on its own would be better than something inline. All it needs are variables for update interval and message, and an update method (mayhaps with optional extra info string); then ReadParser can inherit from it, and it can be instantiated on its own when necessary. Also gives a place to stick future logging and debugging output. Unless exactly this was what @ctb means, and if so, disregard :)

Are you thinking this would be best implemented in Python land or C/C++ land? It looks like the ReadParser class is implemented using CPython and C structs, so it's not initially clear to me how we would have ReadParser inherit from a minimal "progress indicator" class. I know inheritance and polymorphism are possible in C, but it can get hairy.

Forgive me if I'm missing something obvious. :-/

Bleh, forgot about the unholy mixture of C and C++ code. This would definitely need to be implemented in C/C++ land to not cause a massive performance hit. To have it be in Python land and be a C++ class, the checklist is basically:

• Write the ProgressIndicator class

  ie something like:

  class ProgressIndicator {
      public:
      int update_interval;
      string main_msg;
      std::ostream stream; // probably set this to std::cerr by default
      ProgressIndicator(int update_interval, string main_msg, std::ostream stream);
  
      void update(string extra_msg, std::ostream stream);
      // and so forth...
  }

• Write a CPython wrapper for it in the format of the HashBits etc (ie just a container for a pointer)

• Two options:

  • Use inheritance: Convert ReadParser from a struct to class (should be simple), inherit from ProgressIndicator, update CPython wrapper for ReadParser accordingly
  • Use object composition: Keep ReadParser as struct and add an extra param to its constructor accepting a ProgressIndicator; then update the CPython wrapper to take a PyObject* for the ProgressIndicator, and have it reach in and grab the ProgressIndicator* to pass to the ReadParser constructor. This lets either the user define a python-land ProgressIndicator to pass to ReadParser (and other places) or C++ code to use one directly.

  Object composition seems more flexible and easier to test; after writing this out, leaning toward that. Thoughts @ctb?