debrief / pepys-import

Support library for Pepys maritime data analysis environment
https://pepys-import.readthedocs.io/
Apache License 2.0
5 stars 5 forks source link

Suggestion: pull progress bar up into parent class #229

Closed IanMayo closed 4 years ago

IanMayo commented 4 years ago

The progress bar is currently in the parser class.

It would remove a task from the parser-writer/maintainers if this was done in the parent class. We could do this by moving the loop-through-lines into the parent class, with the parser handling a single line at a time. If the parser needs the content from several lines (such as NMEA parser), the data would be stored in self variables.

Note: the parser would need a start method at the start of a new file to clear out the self variables.

Not all parsers parse line-by-line though. So, the XML parser would still have to work in the currently way, and not parse line-by-line.

robintw commented 4 years ago

I'm wondering whether this would actually make it more complicated for parser writers, particularly as we'd have to write it so that it was possible to get either one line at a time (for 'normal' parsers) or the whole file (for the XML parser, and any other parsers like that).

As an alternative, we could provide a function that they can use in the for loop which does the progress bar etc for us. So they would write something like:

for index, line in self.iterate_lines(file_object)

and the iterate_lines function would do the tqdm and enumerate calls, and provide the results using the yield keyword. For example:

def iterate_lines(self, file_object):
    for index, line in enumerate(tqdm(file_object.lines()), 1):
        yield index, line

How does that sound? It makes it easier for the user, but doesn't actually change the interface - so we don't have to support two ways of interacting with parsers.

IanMayo commented 4 years ago

XML yes - I was aware that this didn't iterate line by line.

I did imagine the client classes could override the parent at different levels, something like this:

Importer:
    def load_this_file:

    def _load_this_file:
       # loop through lines
       _load_this_line()
GPX Importer:
   # override file loader
   def _load_this_file:
      # include progress-bar
REP Importer
   # override line loader
   def _load_this_line: 
robintw commented 4 years ago

Fixed in #252