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

Added black as code formatter #530

Merged
merged 5 commits into from
Nov 9, 2022
Merged

Added black as code formatter #530

merged 5 commits into from
Nov 9, 2022

Conversation

kazqvaizer
Copy link
Contributor

refs #529

@kazqvaizer kazqvaizer force-pushed the add-formatter-option branch from c9df5db to d86803c Compare November 6, 2022 14:14
@kazqvaizer kazqvaizer requested a review from f213 November 6, 2022 14:18
@f213
Copy link
Member

f213 commented Nov 6, 2022

Очень круто! А почему хочешь делать форматтер конфигурируемым? Кажется это усложнит дальшейую поддержку. Почему бы не впилить black сразу и на совсем?

@kazqvaizer
Copy link
Contributor Author

А почему хочешь делать форматтер конфигурируемым

Это как раз я хотел вынести на обсуждение. Начал с универсального варианта в этом PR, где каждый может выбрать свой тулл для автоформатирования. В целом я тоже за то, чтобы оставить один форматтер. Но вдруг нужна дискуссия? )

@f213
Copy link
Member

f213 commented Nov 7, 2022

Мне кажется дискуссия в этом случае только увеличивает затраты на поддержку. Го рубить с плеча!

@kazqvaizer
Copy link
Contributor Author

@f213 сделал black сразу и на совсем. Заменил запятые в темплейтах сразу, чтобы потом не переделывать. Думаю, можно мержить, если нет вопросов

@@ -21,6 +21,9 @@ echo Running initial migrations...
./manage.py migrate

cd ../
echo Apply formatting..
make frmt
Copy link
Member

Choose a reason for hiding this comment

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

А не лучше ли здесь будет написать fmt вместо frmt, по аналогии с gofmt?

@@ -10,6 +10,10 @@ deps:
dev-deps: deps
pip-compile --extra=dev --output-file=dev-requirements.txt pyproject.toml

frmt:
cd src && isort .
Copy link
Member

Choose a reason for hiding this comment

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

А разве isort не нужны какие-то параметры, чтобы он форматировал импорты без подтверждения от пользователя?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Не нужно

@@ -82,6 +86,8 @@ ignore = [
"PT001", # Use @pytest.fixture() over @pytest.fixture
"SIM102", # Use a single if-statement instead of nested if-statements
"SIM113", # Use enumerate instead of manually incrementing a counter
"E203", # whitespace before ':', disabled for black purposes https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#slices
"C812", # Missing trailing comma, disabled for black purposes https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#the-magic-trailing-comma
Copy link
Member

Choose a reason for hiding this comment

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

Скажи, а остаётся ли какой-нибудь смысл в наличии flake8-commas теперь?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Интересный вопрос. Пару месяцев назад я думал, что еще нужен, тк были еще проверки C818 C819

Сейчас решил перечитать доки и оказывается, что flake8-commas устарел PyCQA/flake8-commas#69, причем в пользу black PyCQA/flake8-commas#63. Удалю эту батарейку

@@ -7,4 +7,4 @@ def __init__(self, *args, **kwargs):
pass

def run_tests(self, *args):
raise CommandError('Please use command `pytest`.')
raise CommandError("Please use command `pytest`.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise CommandError("Please use command `pytest`.")
raise CommandError("Pytest here. Run it with `make test`")

@f213
Copy link
Member

f213 commented Nov 8, 2022

На всякий случай — мне ок, а у тебя вроде бы есть право мёрджить

@kazqvaizer kazqvaizer changed the title Added configurable formatter option Added black as code formatter Nov 9, 2022
@kazqvaizer kazqvaizer force-pushed the add-formatter-option branch from 2052175 to cdc75fc Compare November 9, 2022 02:43
@kazqvaizer kazqvaizer merged commit ae65c7a into master Nov 9, 2022
@kazqvaizer kazqvaizer deleted the add-formatter-option branch November 9, 2022 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants