karlicoss / ghexport

Export your Github activity: events, repositories, stars, etc.
MIT License
48 stars 5 forks source link

kython import warning #3

Closed purarue closed 4 years ago

purarue commented 4 years ago

Hi. Been modifying/setting up HPI. the major functionality of ghexport works great, but the kython LazyLogger doesn't import properly.

Warning:

./dal.py --source ~/data/github/ghexport/20200826T040221Z.json --interactive
ERROR:root:No module named 'kython'
Traceback (most recent call last):
  File "/home/sean/Repos/ghexport/dal_helper.py", line 100, in logger
    from kython.klogging import LazyLogger # type: ignore
ModuleNotFoundError: No module named 'kython'
WARNING:root:fallback to default logger!

I think thats because its referencing the kython instead of my.kython? Since that part is vendored now, and its more likely someone has the HPI version and not kython on its own, would it make sense to change:

diff --git a/dal_helper.py b/dal_helper.py
index 732ffe8..f259482 100644
--- a/dal_helper.py
+++ b/dal_helper.py
@@ -97,7 +97,7 @@ def logger(logger, **kwargs):
     # TODO FIXME vendorize
     try:
         # pylint: disable=import-error
-        from kython.klogging import LazyLogger # type: ignore
+        from my.kython.klogging import LazyLogger # type: ignore
     except ModuleNotFoundError as ie:
         import logging
         logging.exception(ie)

That seems to work fine for me.

Not sure if you've come up with some other solution for all the boiletplate-y stuff in the different projects

Thanks for all you put up here and your blog, its been very helpful to me over the past few months.

karlicoss commented 4 years ago

Hi, thanks for your kind words! :)

So, the change you propose works, but it would introduce a circular dependency, someone could use ghexport without using HPI package. I am doing a bout of simplification of exporters code soon (to allow them to be proper Python packages, etc), so will most likely remove the remaining bits of kython stuff altogether. This logging thing is not too complicated, so ok to vendorize as well.

But thanks, if you have any other suggestions, let me know! Glad when someone actually digs into the code :)

purarue commented 4 years ago

Makes sense, thanks for the context.