Skip to main content
2 of 2
added 14 characters in body
J_H
  • 34.9k
  • 3
  • 32
  • 129

Thank you for docstrings! You have clearly devoted effort to them - consider pointing to an URL of sphinx output.

In tables, errors, and

@property
def warnings( self ):
    return [] if self._warnings is None else self._warnings

I don't understand why we don't simply return self._warnings, given that __init__ assigned []. Also, PEP-8, no blanks around self. Running flake8 would tidy that up.

I think your recent edit added these four assignments:

class database(object):
    _errors = None
    _warnings = None
    _tables = None
    _rows = None

It would be helpful to add a comment mentioning the __new__ vs. __init__ distinction you're going for.

    ... The value in the dictionary will be another dictionary containing
    'error' (an error message stating exactly what the problem is) and 'foreign_key'
    (the actual foreign key definition that cannot be fulfilled)

That is a perfectly nice public API, using the flexibility of a dict. A more natural API might return a pair (a tuple) instead of a 2nd dict.

    """ Returns False or a string describing a 1215 error message found for the given table and constraint

This also is a clear API, but returning None would be more pythonic. Elsewhere you have several trinary functions that very nicely return True / False / None - that makes perfect sense.

Pair of copy-n-paste typos: "unsigned mistmatch"

The pattern of returning a formatted error string is good, and the one-parameter-per-line formatting is easy to read. There is enough copy-n-paste duplication there to motivate extracting a helper function or two, for some error string fragments.

    # if the column the constraint is on doesn't have an index, then 1215
    index_found = False
    for index in foreign_table.indexes.values():
        if index.columns[0] != foreign_column.name:
            continue
        index_found = True
        break

This seems verbose. Consider building a set() and testing set membership.

This tool seems intended for repeated quick runs over a schema, as it may bail out early upon detecting first error, rather than accumulating a batch of all errors and reporting them together.

In your One Specific Question you wondered whether a certain design decision was the right one. My vote is yes, you made a good choice. It causes the method to become slightly on the long side, but that's fine here. It has clear repetitive logic, and it is good to see all the tests together. At some point a maintainer will likely have to add Yet Another Test for MariaDB 15, and it will be perfectly clear where to start reading, and where to add that Nth test. Good job!

J_H
  • 34.9k
  • 3
  • 32
  • 129