SUPP: Pin back version of isort #2079
Conversation
|
I opened a follow up issue for this: #2080 |
|
@xmnlab can you give an example demonstrating the problem? Why not take the release version of isort and install only using conda (not in pip section) ? |
|
@anmyachev you can run locally |
|
let me know if you could reproduce the error locally. |
|
IIRC the conflict is with adding new lines before comments in the import section ( |
.isort.cfg
Outdated
| @@ -1,3 +1,7 @@ | |||
| [settings] | |||
| known_third_party = asv,click,clickhouse_driver,dateutil,google,graphviz,impala,jinja2,kudu,multipledispatch,numpy,pandas,pkg_resources,plumbum,psycopg2,pyarrow,pydata_google_auth,pygit2,pymapd,pymysql,pyspark,pytest,pytz,regex,requests,ruamel,setuptools,sphinx_rtd_theme,sqlalchemy,toolz | |||
| ensure_newline_before_comments=true | |||
| line_length = 79 | |||
There was a problem hiding this comment.
hmm, I think these should all be in setup.cfg which is the common place, why are we moving here?
There was a problem hiding this comment.
I moved that because isort was already using that .. so just to keep all configurations in just one file
but I check and setup.cfg also is supported: https://github.com/asottile/seed-isort-config/blob/master/seed_isort_config.py#L19
I will try to move all this to setup.cfg
| @@ -53,5 +52,6 @@ dependencies: | |||
| - xorg-libxpm | |||
| - xorg-libxrender | |||
| - pip: | |||
| # see .pre-commit-config.yaml, we pin to specific versions of black and isort | |||
| # see .pre-commit-config.yaml, isort pinned | |||
There was a problem hiding this comment.
really we need to pin like this?
There was a problem hiding this comment.
for now I think we need it because the conflicts.
maybe also we can replace the black checking inside CI for a pre-commit check
I could reproduce this. |
|
@jreback @anmyachev it is ready for a new review! thanks! |
| @@ -56,6 +56,10 @@ black: | |||
| # check that black formatting would not be applied | |||
| black --check . | |||
|
|
|||
| check_pre_commit_hooks: | |||
There was a problem hiding this comment.
please also add next target:
docker_check_pre_commit_hooks: build
# check if all pre-commit hooks are passing inside ibis container
$(DOCKER_RUN) pre-commit run --all-files
So we will be able to use one target in CI and locally.
ci/azure/linux.yml
Outdated
| @@ -62,8 +62,8 @@ jobs: | |||
| - bash: docker-compose run ibis pydocstyle --match-dir="(ibis|omniscidb)" | |||
| displayName: "Docstring check" | |||
|
|
|||
| - bash: make docker_black PYTHON_VERSION=$PYTHON_VERSION | |||
| displayName: 'Ensure black formatting' | |||
| - bash: docker-compose run ibis make check_pre_commit_hooks | |||
There was a problem hiding this comment.
you can use next command: make docker_check_pre_commit_hooks PYTHON_VERSION=$PYTHON_VERSION
|
Thanks @anmyachev for the review. I added your suggestions. |
56df7a2 to
498b8db
Compare
498b8db to
f643f60
Compare
|
@xmnlab maybe the link can help to fix makefile issue: https://stackoverflow.com/questions/16931770/makefile4-missing-separator-stop |
|
thanks @anmyachev for the link! |
1209123 to
f3add9d
Compare
|
ready for review :) |
|
LGTM, @jreback ? |
|
thanks @xmnlab |
|
thanks @anmyachev and @jreback for the review! |
isort 4.3.21 formatting conflicts in some cases with black. I am moving back isort to the previous pinning.
also isort from master has a lot of formatting conflicts with black