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

Стажировка, Haskell: разные уточнения #345

Open
antonkalinin-ml opened this issue Dec 29, 2021 · 7 comments
Open
Labels
backend Related to back-end developer roadmap

Comments

@antonkalinin-ml
Copy link
Contributor

antonkalinin-ml commented Dec 29, 2021

Опыт последнего ревью сервера показал, что требования можно реализовать настолько "в лоб", что пользы от проекта намного меньше, чем могло бы быть, но на ревью к этому трудно придраться. Вот же - в требованиях не написано, значит, необязательно. Заставлять переделывать весь проект не хочется, это во многом наш недочет, что требования можно понять неправильно и проделать массу работы впустую. Нужно ваше мнение по поводу доработок.

Срочное:

  1. убрать упоминание только hindent. В Стажировка, Haskell: лимитировать список форматтеров #344 обсуждается, какой список форматтеров должен быть, но это надолго, а пока надо привести требования в актуальный вид.
  2. написать, что все импорты должны быть либо qualified, либо с импорт листом. (Позже можно обсудить, настолько ли это надо).
  3. добавить к серверу пункт про запрет произвольных либ. Явно написать, какие можно: warp, wai, cryptonite, для конфига, все из haskell platform, библиотека для базы, но без автоматических миграций. У нас есть список, но он лежит в разделе про бота.
  4. добавить в источники ссылку на статью Олега, так как в чате есть вопросы про Handle Pattern. Существующие источники предлагают не париться и писать все хендлы прямо в IO, это немного не то, что мы хотим.
  5. про бот написать, что проект должен быть один, а не два.
  6. В бот и сервер добавить подсказку "перед тем, как делать, прочитай требования в задании 5".

Менее срочное, но важное:

  1. сделать требования для API более детальными, вплоть до названий эндпойнтов, методов и параметров в урле. Формат request body не будем указывать, с этим все справляются. Это нужно, чтобы люди попробовали разные способы передачи параметров (в пути, в квери), а не передавали все через боди в один эндпойнт, включая GET-запросы. Однажды в будущем, может быть, у нас будет постман-коллекция для тестов. У нас упоминается, что нужно использовать принципы REST, но это сложная, полумифическая штука, которая далеко не везде нужна и нарушается где только можно (включая нас), не стоит заставлять людей тратить на это время, а потом переделывать. Лучше дадим готовый образец API, как надо, и этого во многом будет достаточно (на моем последнем проекте - более чем). Недостаток: когда API жестко задано, то все проекты более похожи и есть ощущение, что чужой проект легче выдать за свой (хотя для этого и сейчас нет никаких препятствий, кроме бдительного ревью и собеседований).
  2. картинка должна возвращаться в бинарном виде, с заголовком Content-Type: image/png, либо попросим явно передавать контент-тайп в запросе, хранить его в базе и возвращать. Новость и черновик должны включать в себя урлы картинок. Для создания картинки можно послать ее содержимое в base64 в JSON, чтобы не заморачиваться с multipart/form-data.
  3. выбрать один простой способ авторизации: basic auth (или, может, bearer), никаких куков и прочей экзотики :)
  4. что должно быть можно задать в конфиге:
    • подключение к базе: имя базы, юзер, пароль, хост, порт. Чтобы ревьюер мог создать базу, как ему удобно, а не возиться со скриптами в проекте.
    • минимальный уровень логирования - чтобы посмотреть на дебаг-сообщения, не правя код.
  5. запрос на редактирование сущности должен позволять редактировать любое количество любых полей сущности - любое одно, любые два, все сразу. (Видел, когда реализован только вариант "все сразу" ).
  6. использовать cryptonite - это не требование, а подсказка. Можно и другими способами, например, через постгрес.
  7. должна быть возможна фильтрация новостей одновременно по нескольким критериям и одновременно с сортировкой. Фильтры и сортировку передавать в квери.
  8. вложенные сущности предлагаю слать в ответах везде, а не только в новостях. Это должно побудить переиспользовать код.
  9. Для категории описать формат сущности в API: массив с иерархией категорий, от корня к нужной категории, например [{"category_id": 1, "name": "Programming"}, {"category_id": 5, "name": "Haskell"}]. В базе категории представляются совсем не так, потребуется написать кастомную функцию преобразования.
  10. в требования к ревью дописать, что код должен переиспользоваться, а не копипаститься. Должна быть возможность при помощи единственной правки изменить формат вывода (перечень полей и имена) любой сущности сразу везде.
  11. расписать, что такое юнит-тесты (они не должны задействовать веб и базу), и хорошо бы даже привести обязательные тесткейсы. е2е-тесты не годятся - чтобы их написать, не нужны никакие Handle Pattern/ReaderT, а для юнит-тестов нужны. Только я затрудняюсь с кейсами, основная функциональность это вынуть-положить в базу. Можно добавить следующие требования и попросить, чтобы они были проверены в тестах:
    • все теги должны иметь уникальные имена. Это условие надо проверять при создании и редактировании тега.
    • все категории внутри одного родителя должны иметь уникальные имена. Это нужно проверять при создании и редактировании.
    • при редактировании категории не должно образовываться циклов, т.е. категория не может быть своим собственным родителем или потомком. Проверять при редактировании.
    • пару кейсов на state-based testing, когда надо удостовериться, что в базе, допустим, удаляется именно та категория, которая требуется, но ничего другого. В тестах подменяются вызовы к базе, делается моковая "база", все попытки удаления записываются в IORef и потом проверяются.
  12. в задание 5 добавить пункт "перед отправкой проекта на ревью пробегись по требованиям и проверь, что все сделано". Это сэкономит ревьюеру некоторое время на перепалках по поводу форматтеров, хлинта и разворачивания базы :)

Надо уточнить:

  1. какие условия для удаления сущности? Проще всего не удалять, если сущность используется, но на практике это не очень полезно. Но мне не хочется усложнять требования, задание и без того объемное.
@antonkalinin-ml antonkalinin-ml added the backend Related to back-end developer roadmap label Dec 29, 2021
@olgaklimenko
Copy link
Contributor

olgaklimenko commented Jan 27, 2022

  1. Откомментила в ишью по форматтерам
  2. + На проектах обычно придерживаемся
  3. + Надо найти максимально приближенный к нашим требованиям по использованию библиотек проект и оттуда взять список. Если случайно дадим неполный список начнётся изобретение велосипедов (такое уже встречалось).
  4. + Статья Олега обычно очень помогает, 100% надо добавлять
  5. + Можно сюда вот дописать это требование. Специально уточняла про запуск 1 бота из двух через указание в конфиге, но видимо надо уточнить ещё.
    image
  6. + И перед тем как звать на ревью, перед тем как коммитить новый код после ревью - тоже лучше смотреть в задание 5.
  7. - Тут не очень хочется ограничивать творчество стажёра, хочется чтобы он сам придумал схему АПИ, точно сам разобрался где какой метод использовать и тд. И на ревью (мне) не так сложно проверить и написать ишью, чтобы стажер поразбирался ещё раз. Может достаочно будет добавить хороший ресурс в задание.
    Возможно я просто не встречала проекты стажеров с большими проблемами с АПИ, если есть примеры можно кратенько рассказать что там было.
  8. +
  9. +
  10. +
  11. +- Не знаю, может не стоит усложнять. Пусть хотя бы сделают обновление всего сразу.
  12. + Надо дописать, что подсказка. Если будем список библиотек делать из п.3, то надо будет это учесть.
  13. +
  14. +
  15. +- Тут тоже не хочется усложнять. С одной стороны это разнообразить задание, с другой добавит время на его выполнение и ревью.
  16. +
  17. + Новые требования понравились, как раз чтобы разнообразить тестирование.
  18. +
  19. + Можем сделать тут наиболее мягкое требование, или написать варианты и пусть сами выбирают по желанию (а варианты как раз наведут их на то, чтобы задуматься немного и выбрать).

@stanislav-az
Copy link
Contributor

Может все требования к заданиям перенести в git? Чтобы можно было легко понимать какие требования изменились.

@stanislav-az
Copy link
Contributor

Я согласен со всеми изменениями. Вот кроме этой части в тестах

все попытки удаления записываются в IORef и потом проверяются

Думаю лучше требовать сделать чистые юнит тесты, с простым StateT, чтобы всё было без IO. Тогда не возможно оставить что-то из реальной имплементации которая и работает в IO.

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

@antonkalinin-ml
Copy link
Contributor Author

Может все требования к заданиям перенести в git? Чтобы можно было легко понимать какие требования изменились.

Это надо уговаривать менеджеров по стажировке. Этот вариант уже вроде рассматривался, когда уходили из риззомы, и решение было не в пользу гитхаба :(. Можно перенести только задания, они их не правят, но программа обучения на 90% состоит из заданий, и если их переносить, то в коде останется только введение и FAQ :).

@antonkalinin-ml
Copy link
Contributor Author

Возможно я просто не встречала проекты стажеров с большими проблемами с АПИ, если есть примеры можно кратенько рассказать что там было.

В одной работе видел:

  • GET-запросы с боди. Ни в одном запросе параметры не передавались в URL (в пути или в квери), все через боди.
  • PUT-запрос принимает в request body данные не того формата, в котором возвращает соответствующий GET. Это легко исправить, можно использовать PATCH.
  • раздельные эндпойнты для разных фильтров новостей, например /news/by_tag, /news/by_category, что исключает их использование вместе. Если укажем API точнее, таких ошибок не будет.

Когда я делал сервер, сильно заморачивался, так как не знал, насколько хорошо надо реализовать ресты. Например, при создании тега я мог вернуть 201 Created, если создан новый тег (+ какой-то заголовок с URI этого тега), либо 200 ОК, если тег с таким именем уже есть, и дополнительный заголовок с урлом найденного тега. Все это было не нужно.

@olgaklimenko
Copy link
Contributor

раздельные эндпойнты для разных фильтров новостей, например /news/by_tag, /news/by_category, что исключает их использование вместе. Если укажем API точнее, таких ошибок не будет.
Это нужно в задании уточнить.

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

Если мы предоставим готовую схему АПИ, такие ошибки исчезнут, но никто и не будет пробовать разработать схему самостоятельно. Надо исходить из того насколько джуну-1 надо уметь это делать. Думаю, первому джуну никто не будет поручать такие задачи (хотя тут тоже от проекта зависит).

@antonkalinin-ml
Copy link
Contributor Author

Согласен, нехорошо отбирать разработку схемы. Тогда можно написать перечень требований, что там должно быть: должна быть передача параметров в урле и пр.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to back-end developer roadmap
Projects
None yet
Development

No branches or pull requests

3 participants