cldf / segments

Unicode Standard tokenization routines and orthography profile segmentation
Apache License 2.0
31 stars 13 forks source link

Removes unused logging import and basicConfig. #48

Closed kylebgorman closed 4 years ago

kylebgorman commented 4 years ago

Tokenizer calls basicConfig at module scope, meaning that users who import Tokenizer without having set their own logging first lose the ability to control log level (see #47).

Closes #47.

codecov-io commented 4 years ago

Codecov Report

Merging #48 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #48   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines         394    393    -1     
=====================================
- Hits          394    393    -1
Impacted Files Coverage Δ
src/segments/tokenizer.py 100% <ø> (ø) :arrow_up:
src/segments/__main__.py 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f71fc36...f56acc8. Read the comment docs.

xrotwang commented 4 years ago

I agree that resorting to a function-level import is a bit ugly, but I'd still want to make sure a logging handler is configured when the cli is used. So adding the basicConfig call before https://github.com/cldf/segments/blob/f71fc360fab11f7f1419c6b60e3cc7e3789923fb/src/segments/__main__.py#L43 would be nice.

kylebgorman commented 4 years ago

Good idea, done.

xrotwang commented 4 years ago

@kylebgorman do you need a release on pypi with this change right away, or can we wait until more issues aggregate?

kylebgorman commented 4 years ago

You don't have to do a release, we're using the ugly solution discussed in-issue, for now, though it wouldn't hurt ;)

Thanks for fast review and action!

On Sun, Nov 10, 2019 at 1:22 PM Robert Forkel notifications@github.com wrote:

@kylebgorman https://github.com/kylebgorman do you need a release on pypi with this change right away, or can we wait until more issues aggregate?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cldf/segments/pull/48?email_source=notifications&email_token=AABG4OJARAXCE7JEK4V5JX3QTBGQHA5CNFSM4JLHUEWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDVDJLY#issuecomment-552219823, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4OI5EUVNJSR6KGVTMFTQTBGQHANCNFSM4JLHUEWA .