Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pylint_plugins: add forbidden import checker #463

Closed
wants to merge 1 commit into from
Closed

pylint_plugins: add forbidden import checker #463

wants to merge 1 commit into from

Conversation

HonzaCholasta
Copy link
Contributor

@HonzaCholasta HonzaCholasta commented Feb 14, 2017

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.


name = 'ipa'
msgs = {
'E9901': (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use Warning instead of error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

name = 'ipa'
msgs = {
'E9901': (
'Forbidden import %s',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe would be nice to write whihc rule has been applied Forbidden import %s (%s should not import %s)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

@MartinBasti
Copy link
Contributor

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?

@HonzaCholasta
Copy link
Contributor Author

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

@MartinBasti
Copy link
Contributor

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

@HonzaCholasta
Copy link
Contributor Author

I don't know what you mean, could you give me an example?

@MartinBasti
Copy link
Contributor

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

@HonzaCholasta
Copy link
Contributor Author

You can, using:

    ipaclient/install/

@MartinBasti
Copy link
Contributor

Awesome then

@HonzaCholasta
Copy link
Contributor Author

The format could be nicer though - suggestions are welcome.

@MartinBasti MartinBasti self-assigned this Feb 14, 2017
@martbab
Copy link
Contributor

martbab commented Mar 10, 2017

@MartinBasti any progress in reviewing this PR?

@HonzaCholasta
Copy link
Contributor Author

@martbab, I haven't incorporated @MartinBasti's suggestions in yet.

@HonzaCholasta
Copy link
Contributor Author

I have now incorporated the suggestions.

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.
@MartinBasti MartinBasti added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Mar 10, 2017
@MartinBasti
Copy link
Contributor

master:

  • 5d489ac pylint_plugins: add forbidden import checker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
3 participants