Плохая практика Python: конкретный случай

Многие люди читают о хорошей практике Python, и есть много информации об этом в Интернете. Многие советы включены в книгу, которую я написал в этом году, «Руководство хакера по Python» . Сегодня я хотел бы показать конкретный пример кода, который я не считаю современным.

В моей последней статье, где я рассказывал о своем новом проекте Gnocchi, я писал о том, как я тестировал, взламывал, а затем бросал шепотом . Здесь я собираюсь объяснить часть моего мыслительного процесса и несколько вещей, которые удивили меня при взломе этого кода.

Прежде чем я начну, пожалуйста, не поймите дух этой статьи неправильно. Это никоим образом не является личной атакой на авторов и авторов (которых я не знаю). Кроме того, « шепот» — это фрагмент кода, который создается тысячами установок и хранит метрики годами. Хотя я могу утверждать, что считаю, что код не соответствует передовому опыту, он определенно работает достаточно хорошо и достоин многих людей.

тесты

Первое, что я заметил при попытке взломать шепотом , это отсутствие теста. Есть только один файл, содержащий тесты, имя test_whisper.pyи охват, который он обеспечивает, довольно низкий. Это можно проверить с помощью инструмента покрытия .

$ coverage run test_whisper.py
...........
----------------------------------------------------------------------
Ran 11 tests in 0.014s
 
OK
$ coverage report
Name           Stmts   Miss  Cover
----------------------------------
test_whisper     134      4    97%
whisper          584    227    61%
----------------------------------
TOTAL            718    231    67%

Хотя можно было бы подумать, что 61% — это не так уж и плохо, быстрый взгляд на реальный код теста показывает, что тесты не завершены. Почему я подразумеваю под неполным, что они, например, используют библиотеку для хранения значений в базе данных, но они никогда не проверяют, могут ли результаты быть получены и получены ли результаты точно. Вот веская причина, по которой никогда не следует слепо доверять проценту покрытия теста как метрике качества.

Когда я попытался изменить шепот , так как тесты не проверяли весь цикл значений, введенных в базу данных, я закончил тем, что делал неправильные изменения, но тесты все еще проходили.

Нет PEP 8, нет Python 3

Кодекс не соответствует PEP 8. Пробег flake8 + взлом показывает 732 ошибки … Хотя это не влияет на сам код, это более болезненно взломать на него , чем на большинстве проектов Python.

Инструмент для взлома также показывает, что код не готов к работе с Python 3, поскольку используется только синтаксис Python 2.

Хороший способ исправить это — настроить tox и добавить несколько целей для проверок PEP 8 и Python 3. Даже если набор тестов не завершен, начиная с запуска flake8 без ошибок и нескольких модульных тестов, работающих с Python 3, следует представить проект в лучшем свете.

Не использовать идиоматический Python

Большая часть кода может быть упрощена с помощью идиоматического Python. Давайте рассмотрим простой пример:

def fetch(path,fromTime,untilTime=None,now=None):
  fh = None
  try:
    fh = open(path,'rb')
    return file_fetch(fh, fromTime, untilTime, now)
  finally:
    if fh:
      fh.close()

Этот кусок кода может быть легко переписан как:

def fetch(path,fromTime,untilTime=None,now=None):
  with open(path, 'rb') as fh:
    return file_fetch(fh, fromTime, untilTime, now)

Таким образом, функция выглядит на самом деле настолько простой, что можно даже задаться вопросом, почему она должна существовать, а почему нет.

Использование петель также может быть сделано более Pythonic:

for i,archive in enumerate(archiveList):
  if i == len(archiveList) - 1:
    break

может быть на самом деле:

for i, archive in enumerate(itertools.islice(archiveList, len(archiveList) - 1):

Это уменьшает размер кода и облегчает чтение кода.

Неправильный уровень абстракции

Кроме того, одна вещь, которую я заметил шепотом , это то, что он абстрагирует свои функции на неправильном уровне.

Возьмем create()функцию, это довольно очевидно:

def create(path,archiveList,xFilesFactor=None,aggregationMethod=None,sparse=False,useFallocate=False):
  # Set default params
  if xFilesFactor is None:
    xFilesFactor = 0.5
  if aggregationMethod is None:
    aggregationMethod = 'average'
 
  #Validate archive configurations...
  validateArchiveList(archiveList)
 
  #Looks good, now we create the file and write the header
  if os.path.exists(path):
    raise InvalidConfiguration("File %s already exists!" % path)
  fh = None
  try:
    fh = open(path,'wb')
    if LOCK:
      fcntl.flock( fh.fileno(), fcntl.LOCK_EX )
 
    aggregationType = struct.pack( longFormat, aggregationMethodToType.get(aggregationMethod, 1) )
    oldest = max([secondsPerPoint * points for secondsPerPoint,points in archiveList])
    maxRetention = struct.pack( longFormat, oldest )
    xFilesFactor = struct.pack( floatFormat, float(xFilesFactor) )
    archiveCount = struct.pack(longFormat, len(archiveList))
    packedMetadata = aggregationType + maxRetention + xFilesFactor + archiveCount
    fh.write(packedMetadata)
    headerSize = metadataSize + (archiveInfoSize * len(archiveList))
    archiveOffsetPointer = headerSize
 
    for secondsPerPoint,points in archiveList:
      archiveInfo = struct.pack(archiveInfoFormat, archiveOffsetPointer, secondsPerPoint, points)
      fh.write(archiveInfo)
      archiveOffsetPointer += (points * pointSize)
 
    #If configured to use fallocate and capable of fallocate use that, else
    #attempt sparse if configure or zero pre-allocate if sparse isn't configured.
    if CAN_FALLOCATE and useFallocate:
      remaining = archiveOffsetPointer - headerSize
      fallocate(fh, headerSize, remaining)
    elif sparse:
      fh.seek(archiveOffsetPointer - 1)
      fh.write('\x00')
    else:
      remaining = archiveOffsetPointer - headerSize
      chunksize = 16384
      zeroes = '\x00' * chunksize
      while remaining > chunksize:
        fh.write(zeroes)
        remaining -= chunksize
      fh.write(zeroes[:remaining])
 
    if AUTOFLUSH:
      fh.flush()
      os.fsync(fh.fileno())
  finally:
    if fh:
      fh.close()

Функция делает все : проверяет, не существует ли уже файл, открывает его, строит структурированные данные, записывает это, строит больше структуры, затем записывает это и т. Д.

Это означает, что вызывающая сторона должна указать путь к файлу, даже если она просто хочет, чтобы структура данных whipser хранилась в другом месте. StringIO()может использоваться для фальсификации обработчика файла, но он потерпит неудачу, если вызов fcntl.flock()не отключен — и в любом случае он неэффективен.

В коде много других функций, таких как, например , которые смешивают обработку файлов — даже делают что-то подобное — при манипулировании структурированными данными. Это определенно не очень хороший дизайн, особенно для библиотеки, поскольку оказывается, что повторное использование функции в другом контексте практически невозможно. setAggregationMethod() os.fsync()

if os.path.exists(path):
  raise InvalidConfiguration("File %s already exists!" % path)
fh = None
try:
  # TOO LATE I ALREADY CREATED THE FILE IN ANOTHER PROCESS YOU ARE GOING TO
  # FAIL WITHOUT GIVING ANY USEFUL INFORMATION TO THE CALLER 🙁
  fh = open(path,'wb')

try:
  fh = os.fdopen(os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL), 'wb')
except OSError as e:
  if e.errno = errno.EEXIST:
    raise InvalidConfiguration("File %s already exists!" % path)

def file_fetch(fh, fromTime, untilTime, now = None):
  header = __readHeader(fh)
[...]

def __readHeader(fh):
  info = __headerCache.get(fh.name)
  if info:
    return info
 
  originalOffset = fh.tell()
  fh.seek(0)
  packedMetadata = fh.read(metadataSize)
 
  try:
    (aggregationType,maxRetention,xff,archiveCount) = struct.unpack(metadataFormat,packedMetadata)
  except:
    raise CorruptWhisperFile("Unable to read header", fh.name)
[...]

import StringIO
 
packedMetadata = <some source>
fh = StringIO.StringIO(packedMetadata)
fh.name = "myfakename"
header = __readHeader(fh)

def fetch(path,fromTime,untilTime=None,now=None):
  """fetch(path,fromTime,untilTime=None)
[...]
"""
 
def create(path,archiveList,xFilesFactor=None,aggregationMethod=None,sparse=False,useFallocate=False):
  """create(path,archiveList,xFilesFactor=0.5,aggregationMethod='average')
[...]
"""