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

Correct or disable pylint warnings #72

Merged
merged 1 commit into from
May 25, 2018
Merged

Conversation

guewen
Copy link
Member

@guewen guewen commented May 25, 2018

The remaining warning is fixed in #71 and a warning is fixed in the checker at OCA/pylint-odoo#201

Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

LGTM

@guewen guewen force-pushed the 11.0-pylint-warnings branch from 7bb141a to e69bfcc Compare May 25, 2018 11:26
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

That's indeed a prerequisite for moving to Mature :)

@guewen guewen mentioned this pull request May 25, 2018
# copyright 2016 Camptocamp
# license agpl-3.0 or later (http://www.gnu.org/licenses/agpl.html)

from datetime import datetime, date
import json

from odoo.tests import common

# pylint: disable=odoo-addons-relative-import
# we are testing, we want to test as we were an external consumer of the API
from odoo.addons.queue_job.fields import JobEncoder, JobDecoder
Copy link
Member Author

Choose a reason for hiding this comment

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

That's maybe not the place to discuss that, but am I the only with the opinion that tests should run as if they were from the outside, so should use absolute imports rather than relative?

Copy link
Member

Choose a reason for hiding this comment

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

@guewen you should put instead this import:

from ..fields import JobEncoder, JobDecoder

That's why pylint complains

Copy link
Member

Choose a reason for hiding this comment

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

@guewen has a point

Copy link
Member

Choose a reason for hiding this comment

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

Is there any actual case where the relative import would fail?

Copy link
Contributor

Choose a reason for hiding this comment

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

I find the point of view of @guewen really relevant.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but with no case, I prefer to be homogeneous across all code, being test or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

you should put instead this import:

from ..fields import JobEncoder, JobDecoder

That's why pylint complains

I know, my point is that in tests, I don't want to do that (I do in in addons code for sure).

@pedrobaeza It's not the question of failure, both should work in any case.
It's more a question of what a test is and what the boundaries of the addons are.

If we say that an addon is a library, the tests are definitely not part of this library, they just happen to be in the same folder but they don't belong to the Python package.

Copy link
Member

Choose a reason for hiding this comment

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

Well, I don't find all that conceptual separation needed. As said, I prefer homogeneity, but as always, maintainers decide and this can be raised for a general discussion for removing that check on tests.

@guewen guewen force-pushed the 11.0-pylint-warnings branch from e69bfcc to 57bb9ab Compare May 25, 2018 12:07
@guewen
Copy link
Member Author

guewen commented May 25, 2018

Rebased from 11.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants