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

Remove future statements #3194

Merged
merged 5 commits into from
Aug 9, 2019
Merged

Remove future statements #3194

merged 5 commits into from
Aug 9, 2019

Conversation

max-sixty
Copy link
Collaborator

Not sure why these weren't picked up in some of the previous python3-ifying

@Zac-HD
Copy link
Contributor

Zac-HD commented Aug 8, 2019

In all seriousness, pyupgrade will do this and more automatically with the --py3-only option. I'd add it to CI!

@max-sixty
Copy link
Collaborator Author

We did run it through! #3190

But not with the --py3-only it seems... We could do that in this PR

One reason not to enforce it - I can't seem to work out how to apply it to all files in a repo - am I missing something basic? Or does it need another process to loop over files?

@Zac-HD
Copy link
Contributor

Zac-HD commented Aug 8, 2019

The author pushes pre-commit pretty hard, but that shouldn't be a problem for Xarray as we're already using it. Then pre-commit run --all-files will execute it.

@max-sixty
Copy link
Collaborator Author

max-sixty commented Aug 9, 2019

I think pre-commit is great, but that it's too much burden to compel new contributors to use by adding its effects to CI checks.
I think we've been fairly successful recently at having some less technical folks contribute; keen not to erect even small barriers.

We could add this to pre-commit but not to CI; there'd be some unrelated changes in PRs though.

If anyone is up for a side project - a tool that submits PRs whenever someone commits non-idiomatic code!

Open minded as ever though

@max-sixty
Copy link
Collaborator Author

One addendum: if pre-commit gained widespread use, and new contributors were installing and learning about it early, I would definitely be up for loading that up with checks

@Zac-HD
Copy link
Contributor

Zac-HD commented Aug 9, 2019

My config uses tox and bash instead of pre-commit. Doesn't work well for Windows, but it's a tiny Python script to glob for .py files and subprocess.run the pyupgrade command. Balancing powerful CI against new-contributor-friendliness is hard!

[testenv:check]
deps =
    -r requirements.txt
whitelist_externals = bash
commands =
    autoflake --recursive --in-place --remove-all-unused-imports --remove-duplicate-keys --remove-unused-variables .
    bash -c \"pyupgrade --py36-plus **.py\"
    isort --recursive --apply .
    black .
    flake8
    mypy --config-file=tox.ini . src/
    bandit --recursive --exclude=./.tox/** --skip=B101,B110,B310 --quiet .

@max-sixty
Copy link
Collaborator Author

Nice! That's quite a list. Between us there's an xkcd or narkoz reference...

@max-sixty max-sixty merged commit 58001d7 into pydata:master Aug 9, 2019
@max-sixty max-sixty deleted the future branch August 9, 2019 04:33
dcherian added a commit to dcherian/xarray that referenced this pull request Aug 9, 2019
* upstream/master:
  Ignore example.grib.0112.idx file (pydata#3198)
  small updates to the contributing.rst (it could use more) (pydata#3193)
  Remove future statements (pydata#3194)
  update instructions (pydata#3195)
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