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 php vardump checker #4

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pyatnitsev
Copy link

I add JakubOnderka\PhpVarDumpCheck integration, please take a look

@pyatnitsev pyatnitsev changed the title Feature/add php vardump checker Add php vardump checker Oct 17, 2019
@AntonAcc
Copy link
Contributor

В доке либы не нашел, на какие кейсы реагирует и что считает нормальным. Хотелось бы где-то это увидеть что бы показать разработчикам как можно, а как теперь нельзя.

На каких кейсах сам тестировал?

@pyatnitsev
Copy link
Author

А можешь посмотреть реферес в configuration.ini.dist

[phpvardumpcheck]
className='PhpCsBitBucket\Checker\PhpVarDumpCheck'
mode='--symfony'

вот такой новый конфиг добавился. + я добавил его в секцию checkers

Можно задавать mode, у библиотеки в readme описаны допустимые значения. Если не задавать mode - тогда будет проверять на стандартные var_dump, var_export, print_r

Если задать как у меня - то добавятся еще Symfony-дампы - dump().

В текущей версии основной библиотеки не поддерживаются dd() для symfony, запросил выпустить релиз.

@pyatnitsev
Copy link
Author

Тестировал на своем репозитории, примерно такой вызов будет для личного репозитория пользователя в Bitbucjet:

php app.php test ~DPYATNITSEV dkron

где test - ветка, ~DPYATNITSEV - проект (это имя пользователя начинающееся с ~), dkron - название репозитория.

Я сделал следующее:

  • Создал отдельную ветку,
  • Сделал туда commit с var_dump и dump в разных строчках
  • Сделал PR в Master
  • Запустил Tool из-под себя

Получил 2 сообщения - что забыл dump там и там.

@AntonAcc
Copy link
Contributor

AntonAcc commented Oct 17, 2019

Т.е. например https://git.finam.ru/projects/WT/repos/sparta/browse/comon/lib/generic/RequestHandlerSwitcher.php#272 больше не возможно будет закоммитеть?

А как на var_export($var, true) отреагирует?

Беспокоит ложное срабатывание на полезное использование и отсутствие возможности обойти или игнорировать.

@pyatnitsev
Copy link
Author

Там можно задавать исключения, в виде путей на файлы,
Могу это добавить в саму либу,

но как отличить хорошее срабатывание от плохого - можно подумать

@pyatnitsev
Copy link
Author

Можно добавить разрешенные функции - и добавить туда как раз var_export и обработать это на стороне тула

@AntonAcc
Copy link
Contributor

AntonAcc commented Oct 17, 2019

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

@pyatnitsev
Copy link
Author

Добавил возможность исключать конкретные функции из проверки.

То, что я добавил - дополнительный функционал - его можно включить/выключить в конфиге.

Какое согласование с командой WTT требуется?

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.

None yet

2 participants