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

Feat/guard clause #19

Merged
merged 26 commits into from
Jan 10, 2025
Merged

Feat/guard clause #19

merged 26 commits into from
Jan 10, 2025

Conversation

TorinAsakura
Copy link
Member

#18

@TorinAsakura TorinAsakura self-assigned this Jan 7, 2025
@TorinAsakura TorinAsakura marked this pull request as draft January 7, 2025 21:38
@TorinAsakura TorinAsakura marked this pull request as ready for review January 8, 2025 21:16
@Nelfimov Nelfimov assigned OsirisAnubiz and unassigned TorinAsakura Jan 9, 2025
Copy link
Member

@Nelfimov Nelfimov left a comment

Choose a reason for hiding this comment

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

дописать тесты

@OsirisAnubiz
Copy link
Contributor

OsirisAnubiz commented Jan 9, 2025

@Nelfimov @TorinAsakura

Думаю стоит изменить AbstractGuardExtensionFactory

  1. Лишние параметры передаются. Зачем передавать target ,methodName, paramIndex? Туда нужно просто передавать некий token, чтобы он был менее связан с GuardFactory.
  2. В текущей реализации вообще нет смысла в передавании даже token в методы AbstractGuardExtensionFactory. Там по сути должны храниться Options и логика создания и вызова проверки.

Иными словами AbstractGuardExtensionFactory не соответствует принципам Low Coupling & High Cohesion из-за связанности по параметрам с GuardFactory. Ещё от лишнего Map позволит изваиться

Думаю должно быть скорее:

register(options: AbstractGuardExtensionFactoryOptions): {...}
perform(value: T) {...}

@OsirisAnubiz
Copy link
Contributor

GuardFactory тоже слишком завязан на то что это для декораторов делается.

@Nelfimov
Copy link
Member

Nelfimov commented Jan 9, 2025

@OsirisAnubiz выглядит здраво

@TorinAsakura
Copy link
Member Author

@Nelfimov @TorinAsakura

Думаю стоит изменить AbstractGuardExtensionFactory

  1. Лишние параметры передаются. Зачем передавать target ,methodName, paramIndex? Туда нужно просто передавать некий token, чтобы он был менее связан с GuardFactory.
  2. В текущей реализации вообще нет смысла в передавании даже token в методы AbstractGuardExtensionFactory. Там по сути должны храниться Options и логика создания и вызова проверки.

Иными словами AbstractGuardExtensionFactory не соответствует принципам Low Coupling & High Cohesion из-за связанности по параметрам с GuardFactory. Ещё от лишнего Map позволит изваиться

Думаю должно быть скорее:

register(options: AbstractGuardExtensionFactoryOptions): {...}
perform(value: T) {...}

Согласен. Добивайте да катите уже.

@OsirisAnubiz
Copy link
Contributor

OsirisAnubiz commented Jan 9, 2025

Что сделано:

  • Отрефактрил factory и extensions

Что дальше:

  • Доделать декораторы
  • Написать тесты на базовые декораторы, которые создам
  • Исправить чеки

@TorinAsakura
Copy link
Member Author

Что сделано:

  • Отрефактрил factory и extensions

Что дальше:

  • Доделать декораторы
  • Написать тесты на базовые декораторы, которые создам
  • Исправить чеки

нахера тут тесты?

@OsirisAnubiz
Copy link
Contributor

Что сделано:

  • Отрефактрил factory и extensions

Что дальше:

  • Доделать декораторы
  • Написать тесты на базовые декораторы, которые создам
  • Исправить чеки

нахера тут тесты?

Никита писал, тогда их не буду делать

@OsirisAnubiz
Copy link
Contributor

@TorinAsakura я почему-то не могу запушить после мёрджа.

Пытаюсь запушить, но пишет, что у меня не подписанно при том, что у меня ключ настроен.

@OsirisAnubiz
Copy link
Contributor

Что сделано:

  • Исправил чеки
  • откатился до момента пока не начил рефакторить, ибо уже работает

Что дальше:

  • У меня с мерджом странные проблемы

@TorinAsakura TorinAsakura merged commit ca8ca3b into master Jan 10, 2025
7 checks passed
@TorinAsakura TorinAsakura deleted the feat/guard-clause branch January 10, 2025 19:00
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.

3 participants