wirenboard / wb-mqtt-knx

KNX to MQTT gateway
https://wirenboard.com/wiki/MQTT_KNX
Other
2 stars 0 forks source link

40324 Port wb-mqtt-knx to libwbmqtt1 #5

Closed rkochkin closed 2 years ago

rkochkin commented 3 years ago
webconn commented 3 years ago

Пока всё не смотрел, но сходу:

webconn commented 3 years ago

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

rkochkin commented 3 years ago
  1. Надо комментарии к методам, не всегда ясно, что они делают, что им передавать, могут ли они выкидывать исключения, потокобезопасны ли они.

Планирую сделать в следующих PR. Добавил в Issue

  1. Меня уже тут били по рукам за std::tuple. Лучше возвращать структуру с нормальными именами полей.

Обсудили. Оставил как есть. Функция приватная и маленькая, что-бы делать для неё структуру.

  1. Стоит посмотреть, где можно добавить к методам const.

Добавил, вроде, везде.

  1. Не понял, зачем сделаны интерфейсы IKnxClient и IKnxService?

Удалил IKnxService как излишний. IKnxClient нужен для уменьшения связности и для возможности юнит тестирования. IKnxClient предоставляет высокоуровневый интерфейс для обмена пакетами с сетью KNX.