Discussion:
[Freeipa-devel] [freeipa PR#463][opened] pylint_plugins: add forbidden import checker
HonzaCholasta
2017-02-14 09:40:46 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Author: HonzaCholasta
Title: #463: pylint_plugins: add forbidden import checker
Action: opened

PR body:
"""
Add new pylint AST checker plugin which implements a check for imports
forbidden in IPA. Which imports are forbidden is configurable in pylintrc.

Provide default forbidden import configuration and disable the check for
existing forbidden imports in our code base.

Supersedes @MartinBasti's PR #462.
"""

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/463/head:pr463
git checkout pr463
HonzaCholasta
2017-02-14 10:43:23 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Author: HonzaCholasta
Title: #463: pylint_plugins: add forbidden import checker
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/463/head:pr463
git checkout pr463
MartinBasti
2017-02-14 10:58:32 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

MartinBasti commented:
"""
Can you turn module matching into a regular expression? We need bit more advanced checks, e.g. ipalib should not import from ipaplatform except for modules in ipalib.install.
How can be the issue mentioned by @tiran solved in this PR? should regexp be used or allow rules added?
"""

See the full comment at https://github.com/freeipa/freeipa/pull/463#issuecomment-279676228
HonzaCholasta
2017-02-14 11:01:13 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

HonzaCholasta commented:
"""
@MartinBasti, this issue is already solved in the PR without using regular expressions. See `pylintrc` for example.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/463#issuecomment-279676848
MartinBasti
2017-02-14 11:08:40 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

MartinBasti commented:
"""
Ok, this will not work if ipaclient/submodule allows to import any module, but seems OK for me now, can be improved when needed
"""

See the full comment at https://github.com/freeipa/freeipa/pull/463#issuecomment-279678379
HonzaCholasta
2017-02-14 11:10:17 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

HonzaCholasta commented:
"""
I don't know what you mean, could you give me an example?
"""

See the full comment at https://github.com/freeipa/freeipa/pull/463#issuecomment-279678738
MartinBasti
2017-02-14 11:22:39 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

MartinBasti commented:
"""
In this case:
```
ipaclient/:ipaclient.install:ipalib.install:ipaplatform:ipaserver,
ipaclient/install/:ipaserver,
```

`ipaclient/install` allows all import everything but `ipaserver`, but I cannot currently specify a rule that allows `ipaclient/install` import everything (with `ipaserver`)

But as I said this is a corner case, should be done when needed

"""

See the full comment at https://github.com/freeipa/freeipa/pull/463#issuecomment-279681262
MartinBasti
2017-02-14 11:58:50 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

MartinBasti commented:
"""
Awesome then
"""

See the full comment at https://github.com/freeipa/freeipa/pull/463#issuecomment-279689037
HonzaCholasta
2017-02-14 12:00:11 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

HonzaCholasta commented:
"""
The format could be nicer though - suggestions are welcome.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/463#issuecomment-279689307
HonzaCholasta
2017-02-14 11:57:30 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

HonzaCholasta commented:
"""
You can, using:
```
ipaclient/install/
```
"""

See the full comment at https://github.com/freeipa/freeipa/pull/463#issuecomment-279688754
martbab
2017-03-10 08:12:58 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

martbab commented:
"""
@MartinBasti any progress in reviewing this PR?
"""

See the full comment at https://github.com/freeipa/freeipa/pull/463#issuecomment-285605217
HonzaCholasta
2017-03-10 08:17:36 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

HonzaCholasta commented:
"""
@martbab, I haven't incorporated @MartinBasti's suggestions in yet.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/463#issuecomment-285606048
HonzaCholasta
2017-03-10 11:29:23 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Author: HonzaCholasta
Title: #463: pylint_plugins: add forbidden import checker
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/463/head:pr463
git checkout pr463
HonzaCholasta
2017-03-10 11:30:10 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

HonzaCholasta commented:
"""
I have now incorporated the suggestions.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/463#issuecomment-285646604
MartinBasti
2017-03-10 12:04:42 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

Label: +ack
MartinBasti
2017-03-10 12:05:32 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Author: HonzaCholasta
Title: #463: pylint_plugins: add forbidden import checker
Action: closed

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/463/head:pr463
git checkout pr463
MartinBasti
2017-03-10 12:05:33 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

MartinBasti commented:
"""
master:

* 5d489ac5604ca959cfe439c0594b8739073f3cea pylint_plugins: add forbidden import checker
"""

See the full comment at https://github.com/freeipa/freeipa/pull/463#issuecomment-285652873
MartinBasti
2017-03-10 12:05:34 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/463
Title: #463: pylint_plugins: add forbidden import checker

Label: +pushed

Loading...