@eht16 requested changes on this pull request.

Thanks!
A few remarks:
- `scripts/create_py_tags.py:26: DeprecationWarning: the imp module is 
deprecated in favour of importlib; see the module's documentation for 
alternative uses`scripts/create_py_tags.py:26: DeprecationWarning: the imp 
module is deprecated in favour of importlib; see the module's documentation for 
alternative uses` we should use `importlib` as the message tells
- the `#--------------...` seperators should be either `# --------------...` or 
removed completely (I'm the one who introduced them initially but I'm fine with 
removing them)
- ideally the script is compatible with PEP8

As you said, the resulting `.tags` file should be revised. After a quick look, 
we actually loose some information because the custom `_formatargspec()` 
implementation doesn't work anymore. It errors out with a `TypeError` which we 
silently ignore (I manually checked with `pprint.pformat()` as an example).
I think we should refactor this code to use `inspect.Signature` which was 
probably not available when the current implementation was made (which was more 
than ten years ago). But this is going to be out of scope of this PR, so it's 
fine to fix this later on.

>  # some modules execute funky code when they are imported which we really 
> don't want here
 # (though if you feel funny, try: 'import antigravity')
-PYTHON_LIB_IGNORE_MODULES = (u'antigravity.py', u'idlelib/idle.py', 
u'multiprocessing/util.py')
+PYTHON_LIB_IGNORE_MODULES = ('antigravity.py', 'idlelib/idle.py', 
'multiprocessing/util.py')

Additionally, we should exclude `idlelib/__main__.py` as otherwise the Idle IDE 
is started.
And we could even exclude `this.py` as it contains only the Zen of Python but 
no tags. So we save us printing the Zen of Python.

>  
 PYTHON_LIB_DIRECTORY = os.path.dirname(os.__file__)
-PYTHON_LIB_IGNORE_PACKAGES = (u'test', u'dist-packages', u'site-packages', 
'Tools')
+PYTHON_LIB_IGNORE_PACKAGES = ('test', 'dist-packages', 'site-packages', 
'Tools')

If we also exclude the `config-3.9-x86_64-linux-gnu` package (which might be 
named differently per system), we avoid an irritating usage message:
`Usage: scripts/create_py_tags.py 
[--prefix|--exec-prefix|--includes|--libs|--cflags|--ldflags|--extension-suffix|--help|--abiflags|--configdir|--embed]`

We can read the config module name and strip the prefix part of the part to get 
the module name, maybe like so:
```python
# read the python-config path from LIBPL and strip the prefix part
# (e.g. /usr/lib/python3.8/config-3.8-x86_64-linux-gnu gets 
config-3.8-x86_64-linux-gnu)
PYTHON_CONFIG_DIR = sysconfig.get_config_var('LIBPL')
PYTHON_CONFIG_PACKAGE = PYTHON_CONFIG_DIR[len(PYTHON_LIB_DIRECTORY)+1:] \
    if PYTHON_CONFIG_DIR.startswith(PYTHON_LIB_DIRECTORY) else PYTHON_CONFIG_DIR
PYTHON_LIB_IGNORE_PACKAGES = ('test', 'dist-packages', 'site-packages', 
'Tools', 
                              PYTHON_CONFIG_PACKAGE)
```

> @@ -293,7 +303,10 @@ def main():
     parser.add_builtins()
 
     for filename in args:
-        parser.process_file(filename)
+        try:
+            parser.process_file(filename)
+        except (SystemExit, ImportError, TypeError):

Why catching `SystemExit` and `ImportError`? They do not occur, at least not 
for me.
The `TypeError` exceptions seem rather like an error we should handle instead 
of silently ignoring.

In either case, I think we should print the error instead of silencing it:
```python
        except TypeError as exc:
            print(f'{exc.__class__.__name__} in {filename}: {exc}')
```
this gives: `TypeError in 
/home/enrico/.pyenv/versions/3.9.0/lib/python3.9/http/cookies.py: can only 
concatenate list (not "tuple") to list`

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/2630#pullrequestreview-530781114

Reply via email to