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

Add pre commit hooks #362

Merged
merged 5 commits into from
Dec 7, 2023
Merged

Add pre commit hooks #362

merged 5 commits into from
Dec 7, 2023

Conversation

DmGorokhov
Copy link
Contributor

No description provided.

@Malcom1986
Copy link

@sgmdlt Посмотри плиз

@sgmdlt
Copy link
Collaborator

sgmdlt commented Nov 7, 2023

Добрый день, подскажите, какую задачу вы решаете этим пр?

@DmGorokhov
Copy link
Contributor Author

Добрый день!
В проект добавляются кастомные pre-commit хуки. Перед каждым коммитом теперь прогоняются не только дефолтные хуки git, но и те, что добавлены в файле конфигурации, добавил следующие:

  • flake8- проверка линтера
  • isort- упорядочивает импорты
  • black- приводит файлы к единому оформлению и стилю
  • несколько стандартных из библиотеки: удаление пробелов, добавление пустой строки в конец файла, контроль размера файла, проверка синтаксиса yaml файлов.
    Для всех новых участников скрипты установятся при разворачивание проекта. Для тех, у кого уже развернут- надо будет после обновления главной ветки обновить зависимости и выполнить одну make команду. Отпишусь в чате по факту принятия ПР.

По итогу профит в том, что большинство typo- подобных ошибок, ошибок линтера будет исправляться на стадии коммита, многое поправится автоматически.

@DmGorokhov
Copy link
Contributor Author

@sgmdlt Добрый день! Какое резюме по данному ПР?

@sgmdlt
Copy link
Collaborator

sgmdlt commented Nov 14, 2023

Салют, дело в том, что в проекте уже стоит линтер как локально, так и в CI. Если вы хотите чтобы еще линтинг (и только линтинг, форматирование добавлять не нужно) был в прехуках, то запускайте в прехуке ту же самую команду, что запускает и локальный линтинг.

Вот как это в другом проекте
https://github.com/hexlet-basics/hexlet-basics/blob/5b901556cbd1891a216b0382dacfb4001d5e6651/package.json#L5C1-L6C1

@DmGorokhov
Copy link
Contributor Author

Добрый день!

  • Можете пояснить почему форматирование не стоит добавлять? Чем чревато, что автоматом проверяться yaml, удаляться пробелы, добавится пустые строки? Мне для понимания интересно.

Тогда получается этот issue закрывать надо без ПР? Тк задача стояла добавить .pre-commit и она не решается, нам он получается не нужен.

@fey
Copy link
Collaborator

fey commented Nov 22, 2023

Можете пояснить почему форматирование не стоит добавлять? Чем чревато, что автоматом проверяться yaml, удаляться пробелы, добавится пустые строки? Мне для понимания интересно.

Это неожиданное поведение. Когда мы запускаем команду линтинга, мы ожидаем, что она проверит код, без его модификации. Если нужно код менять, то лучше делать это явно.

в CI делать модификацию кода - ну такое. Иногда линтер может чето даже и сломать (редко, но бывает), поэтому в CI делать модификации кода чаще всего не стоит.

По поводу прехука. Задача тут в том стояла, что было бы хорошо добавить инструмент, который можно подключить для локальной разработки и он будет запускать хук для проверки линтинга. Есть ли такой инструмент сейчас? Сейчас команды линтинга можно только вручную и на CI запустить, если не ошибаюсь.

@fey fey marked this pull request as draft November 22, 2023 02:37
@DmGorokhov
Copy link
Contributor Author

Привет! Вернулся)
image
image

  • Выше скрины с примерами работы прехуков. Я добавил лишний пробел в конце строки и не добавил пустую строчку в конце файла. Хуки поправили это при попытки коммита, но коммит не сделан, тк внесены правки. Т.е. я могу решить дальше как и что. Т.е. я бы не назвал это неожиданным поведением, тк автоматом файлы даже в коммит не улеатают.
    Мы хуками задаем конретные действия- т.е. удали все лишние пробелы и поставь пустые строчки в файлы, если они не проставлены. И мне не нужно это руками делать. Насчет black(задет стиль еадиный стиль кода)- возможно да, тут больше модификаций. Но опять же это только к файлам коммитам применимо и сразу не улетает.

в CI делать модификацию кода - ну такое- я согласен. Просто я бы не назвал это CI. Это просто локальная автопроверка перед коммитом, которую все равно надо аппрувить- выше описал.

По поводу прехука. Задача тут в том стояла, что было бы хорошо добавить инструмент, который можно подключить для локальной разработки и он будет запускать хук для проверки линтинга. Есть ли такой инструмент сейчас? Сейчас команды линтинга можно только вручную и на CI запустить, если не ошибаюсь.- да есть на линтинг. В прехуке мы можем добавить разыне проверки. Линтинг добавлен. Доавил еще несколько, тк почитал это полезным тоже.

Давайте тогда резюмируем, оставляем только прехук на линтинг? Остальное убираем?

@fey
Copy link
Collaborator

fey commented Dec 6, 2023

Обычно прехуки именно делают проверку кода, без модификации. МОжно сказать, что это наше соглашение на проектах - на прехуках не должны модифицироваться файлы, только проверяться =)

Я бы да, оставил только прекоммит на линтинг. Тогда у нас будет логика такая же, как на других проектах - на коммит или пуш выполняется хук, который можно заигнорить через --no-verify и этот хух только проверяет код, не модифицируя его.

Просто даже с модификацией кода, без коммита можно получить непредсказуемое поведение, когда захочется переключиться на другую ветку или стянуть изменения, но гит это заблокирует тк есть изменения.

modified  description of work of pre-commit hooks in CONTRIBUTING.md
@DmGorokhov DmGorokhov marked this pull request as ready for review December 6, 2023 18:42
@DmGorokhov
Copy link
Contributor Author

Спасибо за комментарии, стало понятнее теперь. Поправил, оставил запуск линтера через make команду, ту же, что локальный линтинг запускает.

@fey fey merged commit 04a7de6 into Hexlet:main Dec 7, 2023
1 check passed
@DmGorokhov DmGorokhov deleted the add_pre_commit_hooks branch December 21, 2023 06:50
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.

4 participants