The Wayback Machine - https://web.archive.org/web/20210711205702/https://github.com/scikit-learn/scikit-learn/pull/20412
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

STY Enables black with experimental_string_processing=true #20412

Merged
merged 2 commits into from Jun 29, 2021

Conversation

@thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Jun 27, 2021

Reference Issues/PRs

Resolves #20365

What does this implement/fix? Explain your changes.

This should be merged with the "rebase and merge" option to keep the two commits separate:

  1. MAINT Adds experimental_string_processing to pyproject.toml
  2. STY Runs black with experimental_string_processing=true
@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Jun 27, 2021

The doc error is being fixed here: #20410

Copy link
Member

@ogrisel ogrisel left a comment

I did a manual review of ~30 files at random and could not spot any issue. It generally improves things a lot. I think we can trust the tests for this change to be correct. Doing a full review of this diff is going to be tedious.

Note: there plenty of cases where the code could be even cleaner / simpler by using f-strings instead of .format() or %-based string interpolation but this can be improved progressively later.

@rth
Copy link
Member

@rth rth commented Jun 28, 2021

Note: there plenty of cases where the code could be even cleaner / simpler by using f-strings instead of .format() or %-based string interpolation but this can be improved progressively later.

Yes, if we are into such automatic code changes, running pyupgrade on the code would perform a migration to f-strings. But then this means that such tooling would also need to be available for people solving PR merge conflicts, so what's a good compromise there remains to be determined. Maybe something we can talk about at the meeting.

@ogrisel
Copy link
Member

@ogrisel ogrisel commented Jun 28, 2021

Edit: #20410 was merged. I think you should rebase this on top of main to make sure CI is 100% green before merging.

Edit2: actually there are conflicts, so the black step should be re-run :)

@thomasjpfan thomasjpfan force-pushed the thomasjpfan:black_string_normalize branch from c5e84b8 to 588e08c Jun 28, 2021
@thomasjpfan thomasjpfan force-pushed the thomasjpfan:black_string_normalize branch from 588e08c to 65378ac Jun 28, 2021
@ogrisel
Copy link
Member

@ogrisel ogrisel commented Jun 29, 2021

@rth shall we merge this one? The CI is green and there are no conflicts but it won't last long I think.

@rth
Copy link
Member

@rth rth commented Jun 29, 2021

Yes, thanks @thomasjpfan !

@rth rth merged commit 3ae7c76 into scikit-learn:main Jun 29, 2021
28 checks passed
28 checks passed
@github-actions
check
Details
@github-actions
triage
Details
@github-actions
Check build trigger
Details
@github-actions
Build wheel for cp${{ matrix.python }}-${{ matrix.platform_id }}-${{ matrix.manylinux_image }}
Details
@github-actions
triage_file_extensions
Details
@github-actions
Source distribution
Details
@github-actions
Upload to Anaconda
Details
@lgtm-com
LGTM analysis: C/C++ No code changes detected
Details
@lgtm-com
LGTM analysis: JavaScript No code changes detected
Details
@lgtm-com
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
@circleci-artifacts-redirector
ci/circleci: doc artifact Link to 0/doc/_changed.html
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
@azure-pipelines
scikit-learn.scikit-learn Build #20210628.69 succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Get Git Commit) Get Git Commit succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linting) Linting succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux py37_conda_openblas) Linux py37_conda_openblas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux ubuntu_atlas) Linux ubuntu_atlas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux32 debian_atlas_32bit) Linux32 debian_atlas_32bit succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux_Runs pylatest_conda_mkl) Linux_Runs pylatest_conda_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Ubuntu_Bionic py37_conda) Ubuntu_Bionic py37_conda succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py37_pip_openblas_32bit) Windows py37_pip_openblas_32bit succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (macOS pylatest_conda_forge_mkl) macOS pylatest_conda_forge_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (macOS pylatest_conda_mkl_no_openmp) macOS pylatest_conda_mkl_no_openmp succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants