ryukinix / mal

MAL: A MyAnimeList Command Line Interface [BROKEN: BLAME MyAnimeList]
https://mal.readthedocs.io
Other
109 stars 9 forks source link

mal is broken #17

Closed alexkubica closed 7 years ago

alexkubica commented 7 years ago

I have pulled from dev branch and now mal seems to be broken, the output:

[alex@archlinux mal]$ python mal/cli.py 
Traceback (most recent call last):
  File "mal/cli.py", line 20, in <module>
    from mal import commands
ImportError: cannot import name 'commands'
[alex@archlinux mal]$ 

Some answers for related issues on stackoverflow suggest it happens because of circular dependencies but it doesn't look like the case here.

ryukinix commented 7 years ago

You can't execute mal that way because the PYTHONPATH doesn't contains the package mal inside. You need ensure that mal is installed on your enviroment. See:

mal git/dev  
(env) ❯ python mal/cli.py 
usage: mal [-h]
           {search,increase,inc,decrease,dec,login,list,config,download,watch}
           ...

MyAnimeList command line client.

positional arguments:
  {search,increase,inc,decrease,dec,login,list,config,download,watch}
                        commands
    search              search an anime
    increase (inc)      increase anime's watched episodes by one
    decrease (dec)      decrease anime's watched episodes by one
    login               save login credentials
    list                list animes
    config              config - not implemented yet
    download            download - not implemented yet
    watch               watch - not implemented yet

optional arguments:
  -h, --help            show this help message and exit
(env) ❯ pip list
appdirs (1.4.0)
decorating (0.5)
mal (0.1.5, /home/lerax/Desktop/workspace/github/mal)
pip (8.1.1)
requests (2.11.1)
setuptools (20.10.1)

You installed the last version of mal? The best way is using symlinks and virtualenv as I use (I think). Try python setup.py develop to get PYTHONPATH ok and execute again.

alexkubica commented 7 years ago

It worked for me before. As you can see here the PATH variable should contain the directory of executed script before other directories, which means mal files are included when importing.

ryukinix commented 7 years ago

This is weird, always this fails to me with different packages. You are executing in a underfolder and this makes sense because mal is treated like a package. But I don't understand how this worked before. After all, commands.py, api.py, color.py or any other file inside of mal directory is pretty the same on that type of issue.

You can show me this working without installing the package on the earlier version?

pip list
git checkout v0.1.5
python mal/cli.py
ryukinix commented 7 years ago

I executed on top of v0.1.5 without mal installed and get the same type of problem:

mal git/tags/v0.1.5  
❯ python mal/cli.py 
Traceback (most recent call last):
  File "mal/cli.py", line 18, in <module>
    from mal.api import MyAnimeList
ImportError: No module named 'mal'

Maybe if we use intra-packages for relative imports this can be possible — like from . import commands, but in general, I avoid to use this. We can found several guides criticizing that type of practices.

http://python-notes.curiousefficiency.org/en/latest/python_concepts/import_traps.html

ryukinix commented 7 years ago

Actually this don't fix that too.

Modify all absolute imports to relative paths using intra-packages:

diff --git a/mal/api.py b/mal/api.py
index ce3258d..51213ea 100644
--- a/mal/api.py
+++ b/mal/api.py
@@ -11,7 +11,7 @@ import re
 import requests

 from xml.etree import cElementTree as ET
-from mal.utils import checked_connection, checked_regex
+from .utils import checked_connection, checked_regex
 from decorating import animated

diff --git a/mal/cli.py b/mal/cli.py
index 739eafb..d2f8c93 100755
--- a/mal/cli.py
+++ b/mal/cli.py
@@ -15,10 +15,10 @@ from operator import itemgetter
 from datetime import date

 # self-package
-from mal.api import MyAnimeList
-from mal.utils import usage, killed
-from mal import color
-from mal import login
+from .api import MyAnimeList
+from .utils import usage, killed
+from . import color
+from . import login

 signal.signal(signal.SIGINT, lambda x, y: killed())

diff --git a/mal/login.py b/mal/login.py
index e358df2..55de706 100644
--- a/mal/login.py
+++ b/mal/login.py
@@ -12,9 +12,9 @@ from os import makedirs
 from configparser import ConfigParser
 from getpass import getpass
 from appdirs import user_config_dir
-from mal.api import MyAnimeList
-from mal import color
-from mal import __name__ as APP_NAME
+from .api import MyAnimeList
+from . import color
+from . import __name__ as APP_NAME

 # variables for proper saving
 APP_FILE = 'myanimelist.ini'
diff --git a/mal/utils.py b/mal/utils.py
index bfe2968..b7a8b7d 100644
--- a/mal/utils.py
+++ b/mal/utils.py
@@ -8,13 +8,12 @@
 #

 import sys
-import mal
 import os
 from functools import wraps
 from sre_constants import error as BadRegexError
 from requests.exceptions import ConnectionError
 from decorating.animation import AnimatedDecorator
-from mal import color
+from . import color

 class Unbuffered(object):

Now trying execute

mal git/tags/v0.1.5*  
❯ python mal/cli.py 
Traceback (most recent call last):
  File "mal/cli.py", line 18, in <module>
    from .api import MyAnimeList
SystemError: Parent module '' not loaded, cannot perform relative import
alexkubica commented 7 years ago

I don't have pip installed, but looks like v0.1.5 works:

[alex@archlinux mal]$ git checkout v0.1.5
Note: checking out 'v0.1.5'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at 3a910ae... Update requirements, fix #6 (#7)
[alex@archlinux mal]$ python mal/cli.py 
Usage: mal [inc/+1 | dec/-1] anime-by-regex
       mal [watching | plan to watch | ... | rewatching]
       mal [list | all]
       mal anime-by-regex

Ex. for increment +1:

    $ mal +1 lain

Ex. for decrement -1:

    $ mal -1 lain

Ex. filtering for section:

    $ mal watching

Ex. search return all anime whose start with s: 

    $ mal ^s

Ex. fetch all list: 

    $ mal list
    $ mal all
    $ mal .+

Hacked by Manoel Vilela <manoel_vilela@engineer.com> | version 0.1.5
[alex@archlinux mal]$ git checkout update_readme
Previous HEAD position was 3a910ae... Update requirements, fix #6 (#7)
Switched to branch 'update_readme'
[alex@archlinux mal]$ python mal/cli.py 
Traceback (most recent call last):
  File "mal/cli.py", line 20, in <module>
    from mal import commands
ImportError: cannot import name 'commands'
[alex@archlinux mal]$ git log
commit b7117be49f2cd133ec0eb33b46598c54b89cbf1a
Merge: c51aedb dac8e12
Author: Manoel Machado <manoel_vilela@engineer.com>
Date:   Thu Oct 20 22:51:18 2016 -0300

    Merge pull request #18 from ryukinix/fix-login-command

    Fix mal.commands.login

commit dac8e12b99ab8925f6b4c1085661318daf45e2d1
Author: Manoel Vilela <manoel_vilela@engineer.com>
Date:   Thu Oct 20 22:19:54 2016 -0300

    Fix mal.commands.login: namespace conflict -> module & function

    The commands.login was calling login.create_credentials without
    import login module and I need import using prefix underscore because
    login is defined as function inside of commands.

commit c51aedb2226c9be33e8f73e1548a6b92434cc9da
Merge: 0e87341 935d277
Author: Manoel Machado <manoel_vilela@engineer.com>
Date:   Wed Oct 19 13:59:46 2016 -0300

    Merge pull request #14 from ryukinix/refactor-tree

    Split cli.py => {commands.py, core.py}

commit 935d27785530461bae23c7fe92901f11e504b768
Author: Manoel Vilela <manoel_vilela@engineer.com>
Date:   Wed Oct 19 10:04:31 2016 -0300

    Split cli.py => {commands.py, core.py}

    This makes much more sense, for now. Remove prefix _command
    for commands and use namespace commands.function to give meaning.

    BTW, all the usage of api.py need be here on mal.core

commit 0e8734164c877af9e216fd2d2e1e196afce34056
Merge: 3a910ae 77f8934
Author: Manoel Machado <manoel_vilela@engineer.com>
Date:   Wed Oct 19 09:44:06 2016 -0300

    Merge pull request #12 from AlexK-IL/parse_args

    Refactor command line interface

commit 77f8934b823ad366f78854c2efd769d29443a6d5
Author: Alex Kubica <alexkubicail@gmail.com>
Date:   Tue Oct 18 22:30:25 2016 +0300

    add not implemented commands

commit cb6face2ea8dbcd096f088b9e8c31b579a3ef0bc
Author: Alex Kubica <alexkubicail@gmail.com>
Date:   Tue Oct 18 22:24:51 2016 +0300
[alex@archlinux mal]$ 
ryukinix commented 7 years ago

wtf! I just don't understand it. You have some suspicion about the cause? The most weird is just works in your machine... this really funny. I cannot replicate the ok usage on v0.1.5 on my system, as you can see in another comments.

alexkubica commented 7 years ago

This is weird, after playing with it a bit I got it to work. I changed line: https://github.com/ryukinix/mal/blob/dev/mal/cli.py#L20 to:

import commands

And line https://github.com/ryukinix/mal/blob/dev/mal/commands.py#L14 to:

import core

Why won't it import from mal?

alexkubica commented 7 years ago

Okay after consulting with a friend I finally got it! The problem was that when I executed cli.py locally, it first searched in the same directory as mentioned here, the problem is that the module mal actually starts one directory before! So there are a few working solutions:

  1. Use import X instead of from mal import X - might not work well after publishing the package.
  2. Run the script with PYTHONPATH variable, for example: PYTHONPATH=. python mal/cli.py - probably the best solution.
  3. Move all files but cli.py into a different mal directory - might not be good too since there will already be 2 mal folders, one inside another.

I think that option 2 solves it well without modifying anything, I could mention it when working on https://github.com/ryukinix/mal/issues/8.

Please close this issue after you see it.

ryukinix commented 7 years ago

Thanks for all research and detailed strategies! Particularly 1 and 3 I think is a trap. I like 2. I didn't know.