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

Enable pylint checks #4092

Merged
merged 9 commits into from
Jun 24, 2020
Merged

Enable pylint checks #4092

merged 9 commits into from
Jun 24, 2020

Conversation

skshetry
Copy link
Member

It was getting ugly, so I decided to disable every checks and start enabling one-by-one that is preferred by us.
If there's no consensus among us to enable particular checks, we should keep it disabled.

Deepsource will be affected by this change (and, might return less error while in transition).

  • ❗ I have followed the Contributing to DVC checklist.

  • 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

  • ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@skshetry skshetry self-assigned this Jun 23, 2020
@efiop
Copy link
Contributor

efiop commented Jun 23, 2020

It was getting ugly, so I decided to disable every checks and start enabling one-by-one that is preferred by us.
If there's no consensus among us to enable particular checks, we should keep it disabled.

@skshetry The problem is that we will forget about them if we don't do it right away. Disabling everything doesn't make any sense, we will miss new features and will never enable them.

@skshetry
Copy link
Member Author

@efiop, No, I will keep making any PRs until we enable the checks that we need. I'd not suggest enabling everything and blacklisting some, at least for now. Let's go the other way round.

@efiop
Copy link
Contributor

efiop commented Jun 23, 2020

No, I will keep making any PRs until we enable the checks that we need. I'd not suggest enabling everything and blacklisting some, at least for now. Let's go the other way round.

So you plan on going through all avaliable checks one-by-one? What if there will be new features in the future? Or do you plan on starting with this and then switch this the other way around and enable everything and disable some?

@skshetry
Copy link
Member Author

@efiop, for the start, I'd love to keep things sane with pylint, and keep a whitelisted list of checks. Otherwise, we would just keep fighting over pylint and it's false-positive.

I'd recommend starting small and postpone the switching decisions for later.
See also: pylint-dev/pylint#3512, pylint-dev/pylint#746

@efiop
Copy link
Contributor

efiop commented Jun 23, 2020

@skshetry We will have to enable everything and blacklist some in the near future, otherwise we will not even know that there is something useful (e.g. new) we can whitelist. I'm fine with starting with the current approach only if we switch in the near future.

@skshetry skshetry marked this pull request as ready for review June 23, 2020 17:13
Comment on lines +16 to +17
- name: Install requirements
run: bash ./scripts/ci/install.sh
Copy link
Member Author

Choose a reason for hiding this comment

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

pylint requires all of the dependencies installed to check for import-error. Could either disable it (which is bad) or, need to install all of the dependency before-hand.

Comment on lines +26 to +32
- repo: local
hooks:
- id: pylint
name: pylint
entry: pylint
language: system
types: [python]
Copy link
Member Author

Choose a reason for hiding this comment

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

pre-commit creates an isolated virtualenv. But, for dynamic analysis such as import-error, pylint requires certain sets of dependencies installed, hence we need to use system's python (eg: from pip-installed one).

@skshetry
Copy link
Member Author

skshetry commented Jun 23, 2020

@efiop, I think we need to at least keep pylint only in Github Actions, or run check-patch in both the travis and GHA to not waste time running tests even if the patch is incorrectly formatted.

@skshetry skshetry requested review from pared, efiop and pmrowla and removed request for pared June 23, 2020 17:18
@skshetry
Copy link
Member Author

@efiop, also I think we could start with this PR. I will follow-up with some PRs tomorrow.

Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

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

I agree with @efiop that, in the end, we should enable all checks that are not explicitly forbidden, but if we don't want to end up with single PR containing all fixes, which will be hard to merge, we should iterate gradually over whitelist until we can make the switch. In that case, we should probably reopen #219 and assign someone until the transition is done.

jobs:
build:

runs-on: ubuntu-latest
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid unpredictible behavior, maybe we should stick to particular version? like 18.04/20.04?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's okay to use the latest one. If it breaks, we will know.

.travis.yml Show resolved Hide resolved
@efiop efiop merged commit b81ace5 into iterative:master Jun 24, 2020
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.

3 participants