Skip to main content
Bounty Ended with 50 reputation awarded by Simone Bronzini
Freudian slip
Source Link
200_success
  • 144.2k
  • 22
  • 188
  • 473

Concept

In many ways, the functionality of this library resembles that of the built-in csv module. The main difference is that here you split by regex rather than on a specific character. I think that the design would be improved by modelling your code after the csv module — for example, by having a separate Reader and DictReader.

The fact that the Reader accepts a filename as input limits the applicability of this code. What if I want to parse data coming from a network stream? It can't be done without first writing to a temporary file.

The field numbering convention is very confusing in my opinion:

"""
- record['$4']  # same as record[3]
"""

record['$0'] doesn't retrieve the original text as I would expect.

You should either give up the AWK-inspired '$4' notation (for which I don't see much value) or fully embrace the one-based column numbering (which does have some precedent in Python regular expressions).

The filter functions make the Parser do much more than parsing, violating the Single Responsibility Principle. In addition, the filtering makes it unclear how record numbering works, or what you mean by the "next" record. I think you would be better off dropping the feature, since Python's generator expressions offer much of the same functionality.

Iterators

Your iterator implementation is more complicated than necessary, and in fact wrong.

Here's how iterators should behave:

>>> words = 'The quick brown fox jumps over the lazy dog'.split()
>>> iter1 = iter(words)
>>> iter2 = iter(words)
>>> next(iter1)
'The'
>>> next(iter1)
'quick'
>>> next(iter1)
'brown'
>>> next(iter2)
'The'

However, if I awkask for two iterators on the same Record, they actually interfere with each other:

>>> from awk import Reader
>>> with Reader('fox.txt') as reader:
...     record = next(reader)
... 
>>> str(record)
'Record($1: The, $2: quick, $3: brown, $4: fox, $5: jumps, $6: over, $7: the, $8: lazy, $9: dog)'
>>> iter1 = iter(record)
>>> iter2 = iter(record)
>>> next(iter1)
('$1', 'The')
>>> next(iter1)
('$2', 'quick')
>>> next(iter1)
('$3', 'brown')
>>> next(iter2)
('$4', 'fox')

To support iteration, you didn't need to write a __next__ method; all you needed was this:

class Record:
    …
    
    def __iter__(self):
        """Return an iterator over the record's keys"""
        return ((key, self._field_dict[key]) for key in self._key_list)

Concept

In many ways, the functionality of this library resembles that of the built-in csv module. The main difference is that here you split by regex rather than on a specific character. I think that the design would be improved by modelling your code after the csv module — for example, by having a separate Reader and DictReader.

The fact that the Reader accepts a filename as input limits the applicability of this code. What if I want to parse data coming from a network stream? It can't be done without first writing to a temporary file.

The field numbering convention is very confusing in my opinion:

"""
- record['$4']  # same as record[3]
"""

record['$0'] doesn't retrieve the original text as I would expect.

You should either give up the AWK-inspired '$4' notation (for which I don't see much value) or fully embrace the one-based column numbering (which does have some precedent in Python regular expressions).

The filter functions make the Parser do much more than parsing, violating the Single Responsibility Principle. In addition, the filtering makes it unclear how record numbering works, or what you mean by the "next" record. I think you would be better off dropping the feature, since Python's generator expressions offer much of the same functionality.

Iterators

Your iterator implementation is more complicated than necessary, and in fact wrong.

Here's how iterators should behave:

>>> words = 'The quick brown fox jumps over the lazy dog'.split()
>>> iter1 = iter(words)
>>> iter2 = iter(words)
>>> next(iter1)
'The'
>>> next(iter1)
'quick'
>>> next(iter1)
'brown'
>>> next(iter2)
'The'

However, if I awk for two iterators on the same Record, they actually interfere with each other:

>>> from awk import Reader
>>> with Reader('fox.txt') as reader:
...     record = next(reader)
... 
>>> str(record)
'Record($1: The, $2: quick, $3: brown, $4: fox, $5: jumps, $6: over, $7: the, $8: lazy, $9: dog)'
>>> iter1 = iter(record)
>>> iter2 = iter(record)
>>> next(iter1)
('$1', 'The')
>>> next(iter1)
('$2', 'quick')
>>> next(iter1)
('$3', 'brown')
>>> next(iter2)
('$4', 'fox')

To support iteration, you didn't need to write a __next__ method; all you needed was this:

class Record:
    …
    
    def __iter__(self):
        """Return an iterator over the record's keys"""
        return ((key, self._field_dict[key]) for key in self._key_list)

Concept

In many ways, the functionality of this library resembles that of the built-in csv module. The main difference is that here you split by regex rather than on a specific character. I think that the design would be improved by modelling your code after the csv module — for example, by having a separate Reader and DictReader.

The fact that the Reader accepts a filename as input limits the applicability of this code. What if I want to parse data coming from a network stream? It can't be done without first writing to a temporary file.

The field numbering convention is very confusing in my opinion:

"""
- record['$4']  # same as record[3]
"""

record['$0'] doesn't retrieve the original text as I would expect.

You should either give up the AWK-inspired '$4' notation (for which I don't see much value) or fully embrace the one-based column numbering (which does have some precedent in Python regular expressions).

The filter functions make the Parser do much more than parsing, violating the Single Responsibility Principle. In addition, the filtering makes it unclear how record numbering works, or what you mean by the "next" record. I think you would be better off dropping the feature, since Python's generator expressions offer much of the same functionality.

Iterators

Your iterator implementation is more complicated than necessary, and in fact wrong.

Here's how iterators should behave:

>>> words = 'The quick brown fox jumps over the lazy dog'.split()
>>> iter1 = iter(words)
>>> iter2 = iter(words)
>>> next(iter1)
'The'
>>> next(iter1)
'quick'
>>> next(iter1)
'brown'
>>> next(iter2)
'The'

However, if I ask for two iterators on the same Record, they actually interfere with each other:

>>> from awk import Reader
>>> with Reader('fox.txt') as reader:
...     record = next(reader)
... 
>>> str(record)
'Record($1: The, $2: quick, $3: brown, $4: fox, $5: jumps, $6: over, $7: the, $8: lazy, $9: dog)'
>>> iter1 = iter(record)
>>> iter2 = iter(record)
>>> next(iter1)
('$1', 'The')
>>> next(iter1)
('$2', 'quick')
>>> next(iter1)
('$3', 'brown')
>>> next(iter2)
('$4', 'fox')

To support iteration, you didn't need to write a __next__ method; all you needed was this:

class Record:
    …
    
    def __iter__(self):
        """Return an iterator over the record's keys"""
        return ((key, self._field_dict[key]) for key in self._key_list)
Source Link
200_success
  • 144.2k
  • 22
  • 188
  • 473

Concept

In many ways, the functionality of this library resembles that of the built-in csv module. The main difference is that here you split by regex rather than on a specific character. I think that the design would be improved by modelling your code after the csv module — for example, by having a separate Reader and DictReader.

The fact that the Reader accepts a filename as input limits the applicability of this code. What if I want to parse data coming from a network stream? It can't be done without first writing to a temporary file.

The field numbering convention is very confusing in my opinion:

"""
- record['$4']  # same as record[3]
"""

record['$0'] doesn't retrieve the original text as I would expect.

You should either give up the AWK-inspired '$4' notation (for which I don't see much value) or fully embrace the one-based column numbering (which does have some precedent in Python regular expressions).

The filter functions make the Parser do much more than parsing, violating the Single Responsibility Principle. In addition, the filtering makes it unclear how record numbering works, or what you mean by the "next" record. I think you would be better off dropping the feature, since Python's generator expressions offer much of the same functionality.

Iterators

Your iterator implementation is more complicated than necessary, and in fact wrong.

Here's how iterators should behave:

>>> words = 'The quick brown fox jumps over the lazy dog'.split()
>>> iter1 = iter(words)
>>> iter2 = iter(words)
>>> next(iter1)
'The'
>>> next(iter1)
'quick'
>>> next(iter1)
'brown'
>>> next(iter2)
'The'

However, if I awk for two iterators on the same Record, they actually interfere with each other:

>>> from awk import Reader
>>> with Reader('fox.txt') as reader:
...     record = next(reader)
... 
>>> str(record)
'Record($1: The, $2: quick, $3: brown, $4: fox, $5: jumps, $6: over, $7: the, $8: lazy, $9: dog)'
>>> iter1 = iter(record)
>>> iter2 = iter(record)
>>> next(iter1)
('$1', 'The')
>>> next(iter1)
('$2', 'quick')
>>> next(iter1)
('$3', 'brown')
>>> next(iter2)
('$4', 'fox')

To support iteration, you didn't need to write a __next__ method; all you needed was this:

class Record:
    …
    
    def __iter__(self):
        """Return an iterator over the record's keys"""
        return ((key, self._field_dict[key]) for key in self._key_list)