Skip to content

Banners rotation#23

Open
ezhk wants to merge 25 commits intomasterfrom
banners-rotation
Open

Banners rotation#23
ezhk wants to merge 25 commits intomasterfrom
banners-rotation

Conversation

@ezhk
Copy link
Copy Markdown
Owner

@ezhk ezhk commented Oct 19, 2020

No description provided.

return nil
}

func (q *Queue) RunConsumer(storage *storage.Storage, logger *logger.Logger) error {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Немного странное название метода очереди. Обычно это Push и Pop, а уже консюмер и прод/сер - это некая обёртка.

Copy link
Copy Markdown
Owner Author

@ezhk ezhk Nov 2, 2020

Choose a reason for hiding this comment

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

Просто это не совсем push или pop событие, именно именно инициализация потребителя,
то есть выполняется некоторое Run-событие (если точнее StartConsuming).
Если имеет смысл переименовать, то поправьте меня пожалуйста.

@@ -0,0 +1,5 @@
---
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Для юнит-тестов можно просто создать структуру конфиг и записать в неё нужные данные напрямую в файле с тестом. Файлы конфигов тут лишние.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Там, где вызывается инициализация конфига единожды — поправил на определение конфига через структуру.

queue *queue.Queue
}

// Generate gRPC gateway.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Такое лучше вынести в файл generate.go и положить его в пакет server.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Вынес в generate.go.

@@ -0,0 +1,139 @@
syntax = "proto3";
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Описание апи должно лежать в /api/banner-api.proto, не очевидно потом где нужно искать описание апи.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Описание api и генератор gRPC вынес в API,
основная логика осталась в сервер, но использует структуры API.

db *gorm.DB
}

// Generate base methods for tables.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Убираем генерацию кода в отдельный файл.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Вынес в generate.go.

"gorm.io/gorm"
)

type Storage struct {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Как я понял, везде идёт зависимость на конкретный тип, а не на интерфейс. Это плохо, надо описать интерфейс нашего стореджа, и в зависимостях использовать его. А уже реализация будет вот эта. Почему это хорошо - мы всегда можем заменить наш сторедж на мок или на что-то другое, что реализует нужный интерфейс.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Спасибо, сделал отдельно интерфейс.

@ezhk ezhk requested a review from farir1408 November 2, 2020 13:51

integration-test:
# run tests separate to avoid superposition
go test -race -count 5 -tags integration ./internal/storage/...
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Я бы предложил запускать просто -tags integration ./... Тогда запустятся все тесты с нужным тегом.


lint:
go get github.com/golangci/golangci-lint/cmd/golangci-lint
golangci-lint run --disable exhaustivestruct --disable wrapcheck ./...
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Было бы здорово если бы сделали для линтера конфиг.

@farir1408
Copy link
Copy Markdown
Collaborator

Здравствуйте!

Спасибо за выполнение домашнего задания, хорошая работа.

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

Принято 15/15.

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