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

Style change 2.0 #692

Merged
merged 5 commits into from
Nov 19, 2022
Merged

Style change 2.0 #692

merged 5 commits into from
Nov 19, 2022

Conversation

desh2608
Copy link
Collaborator

@desh2608 desh2608 commented Nov 17, 2022

This is a corrected version of #690.

  • (107df3b) First pass: We apply black formatting without the --experimental-string-processing feature which was causing some files to get deleted entirely.
  • (d31db01) Second pass: I manually went through all the modifications and corrected several of the sort:
            f"expected sample rate: {expected_sample_rate}. "
            f"Given: {sample_rate}"

being converted to:

           f"expected sample rate: {expected_sample_rate}. " f"Given: {sample_rate}"

and a small number of cases where inline comments made the formatting weird.

At the very least, these changes should not cause anything to break. If there is something I missed, I am hoping they can be corrected as people come across them in the future.

@desh2608
Copy link
Collaborator Author

@csukuangfj I would request you to please briefly check the files in the main icefall/ directory since they are used in all recipes, to ensure there are no breaking changes. (You can skip the changes in egs)

python3 -m pip install --upgrade pip black==21.6b0 flake8==3.9.2 click==8.0.4
# See https://github.com/psf/black/issues/2964
# The version of click should be selected from 8.0.0, 8.0.1, 8.0.2, 8.0.3, and 8.0.4
python3 -m pip install --upgrade pip black==22.3.0 flake8==5.0.4 click==8.1.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@csukuangfj
Copy link
Collaborator

Thanks! Looks good to me. Left a minor comment.

@desh2608
Copy link
Collaborator Author

Thanks! Looks good to me. Left a minor comment.

I think it's ready for merge?

@csukuangfj csukuangfj merged commit 500792d into k2-fsa:master Nov 19, 2022
@desh2608 desh2608 mentioned this pull request Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants