OCR-D/ocrd_calamari

Review test/base.py

Closed this issue · 8 comments

mikegerber commented
  • Uses +, not os.path.join()
  • Put exports into __all__ (for e.g. ruff but also to be clean)
  • Shouldn't the exports go to __init__py? (Check other projects for convention)
  • It calls initLogging() but where is test.base itself called? (during import obviously)
  • Sets pylint: disable=unused-import which may be unnecessary with __all__ set
  • ocr_tesserocr and ocr_kranken have a CapturingTestCase that might be interesting
mikegerber commented

I already had added __all__ (because ruff removed assets when it wasn't in __all__ and thus broke test_recognize.py.)

mikegerber commented
  • Uses +, not os.path.join()

I've removed adding ../ocrd to sys.path entirely (#102). Maybe this was some kind of test setup from the early days of OCR-D? (It's also in ocrd_tesserocr and ocrd_kraken)

mikegerber commented 
* [ ]  Sets `pylint: disable=unused-import` which may be unnecessary with `__all__` set

Adding __all__ made pylint (and ruff) happy, so I removed the pylint pragma (#102).

mikegerber commented 
* [ ]  Shouldn't the exports go to `__init__py`? (Check other projects for convention)

I've decided to keep this in test.base now, to be consistent with the other OCR-D projects.

mikegerber commented 
* [ ]  It calls `initLogging()` but where is `test.base` itself called? (during import obviously)

Need to know how to test OCR-D's logging first.

mikegerber commented 
* [ ]  ocr_tesserocr and ocr_kranken have a CapturingTestCase that might be interesting

We already use pytest's caplog fixture.

mikegerber commented 
* [ ]  It calls `initLogging()` but where is `test.base` itself called? (during import obviously)

It bothers me a little that this happens there as a side affect of importing the assets but as it seems to be the convention that it's in here I'm going to leave it for now. Maybe going to discuss it with @kba in the future.

mikegerber commented 
* [ ]  Sets `pylint: disable=unused-import` which may be unnecessary with `__all__` set

Adding __all__ made pylint (and ruff) happy, so I removed the pylint pragma (#102).

@kba Just happened to see the pragma again somewhere in core, while __all__ looked ok there so I'm wondering if I'm mistaken/overlooking something?