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

Extend constructor for signer and logger #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HAPXu3
Copy link

@HAPXu3 HAPXu3 commented Oct 29, 2020

Я подумал, можно расширить конструктор двумя необязательными аргументами для управления инициацией логгера и подписывателя. Поскольку иногда возникает проблема (#31) с подписью через SignerPKCS7, приходится использовать CliSignerPKCS7. Сейчас это можно сделать только через setSigner(), но при этом в конструкторе происходит создание ненужного объекта. Логгер (#30) тоже можно добавить, чтобы, к примеру, можно было настраивать через контейнер зависимостей.

Обратная совместимость не нарушается.

Из минусов только раздувание списка параметров.

@fr05t1k
Copy link
Owner

fr05t1k commented Oct 29, 2020

Привет! Спасибо за предложение 😃

Эти 2 параметры не обязательные. Вы можете их сразу после создания выставить в нужные при помощи сеттеров в том числе при помощи фабрик DI. Я не думаю, что переносить опциональные параметры в конструктор, хорошая идея.

@HAPXu3
Copy link
Author

HAPXu3 commented Oct 29, 2020

Не думаю, что signer опциональный. Тем более он сейчас без вариантов создаётся в конструкторе. Как и логгер. Но да, это скорее дело дизайна 😃

@fr05t1k
Copy link
Owner

fr05t1k commented Oct 29, 2020

Ну если есть поведение по-умолчанию, значит опциональный 😉

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.

2 participants