Skip to content

Версия 17#1

Open
Rarik88 wants to merge 8 commits intomainfrom
dev
Open

Версия 17#1
Rarik88 wants to merge 8 commits intomainfrom
dev

Conversation

@Rarik88
Copy link
Copy Markdown
Owner

@Rarik88 Rarik88 commented Apr 29, 2024

No description provided.

Copy link
Copy Markdown

@dirtymew dirtymew left a comment

Choose a reason for hiding this comment

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

Привет Алекс

хорошо получилось - но нужно доработать

для начала нужно добавить документацию и в ней обязательно описать как запускать приложение и тесты

есть вопросы к нейменгу и архитектуре решения

по организации кода посмотри https://github.com/golang-standards/project-layout
мб наведет на мысли

вопрос - задание со звездочкой делать будешь?

Comment thread cmd/main.go Outdated
)

func main() {
db := hub.Sqlite(con.DB_NAME_SET)
Copy link
Copy Markdown

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.

Добавлена обработка ошибок в ДБ

Comment thread .dockerignore
**/.project
**/.settings
**/.toolstarget
**/.vs
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

нужно добавить файлы .idea

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@Rarik88 а в ветке еще остались
image

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

все еще в ветке @Rarik88

Comment thread config/config.go
@@ -0,0 +1,4 @@
package config
Copy link
Copy Markdown

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.

Буду иметь введу, но в данном случае казалось аккуратнее и уместно

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

это странно - если это конфиг приложение то эти параметры нужны в коменде запуска - зачем они еще?

почему это переменные - а не константы?

если хочется как-то их изменять то надо смотреть или в сторону переменных окружения или аргументов см https://github.com/spf13/viper

Comment thread pkg/api/api.go Outdated
TaskDelete(id string) error
}

type Api struct {
Copy link
Copy Markdown

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.

Изменил

Comment thread pkg/api/api.go Outdated
Task
}

func NewApi(hub hub.Task) *Api {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

мб назвать service, tasks/service - API это про другое скорее про handlers

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.

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

Comment thread pkg/handler/handler.go

r.GET("/api/nextdate", h.ND)

api := r.Group("/api")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

а вот это api и скорее в пакете api я бы ожидал описание эндпоинтов

Comment thread pkg/handler/handler.go
Comment thread pkg/handler/nd.go Outdated
"log"
)

func (h *Handler) ND(c *gin.Context) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

что такое ND - очень плохое название

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.

ND - NextDate, поправил

Comment thread pkg/handler/task.go Outdated
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
return
}
c.JSON(200, gin.H{"id": id})
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

нужно иcпользовать константы а не 200

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.

Замена на http.StatusOK

Comment thread tests/addtask_4_test.go Outdated
"bytes"
"encoding/json"
"fmt"
"github.com/stretchr/testify/assert"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

сломал порядок в импорте - нужно починить

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@Rarik88 тут не поправил

@Rarik88
Copy link
Copy Markdown
Owner Author

Rarik88 commented Apr 30, 2024 via email

Copy link
Copy Markdown

@dirtymew dirtymew left a comment

Choose a reason for hiding this comment

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

Привет Алекс

не увидел изменений по предыдущим комментариям

нет документации, сломаны импорты - это основное что нужно исправить

Comment thread .dockerignore
**/.project
**/.settings
**/.toolstarget
**/.vs
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@Rarik88 а в ветке еще остались
image

Comment thread cmd/main.go Outdated
// добавлена обработка ощибок в Дб
func main() {
db := hub.Sqlite(con.DB_NAME_SET)
if db == nil {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ну вот не добавил - тут бы не на db == nil проверять а на err != nil

Comment thread config/config.go
@@ -0,0 +1,4 @@
package config
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

это странно - если это конфиг приложение то эти параметры нужны в коменде запуска - зачем они еще?

почему это переменные - а не константы?

если хочется как-то их изменять то надо смотреть или в сторону переменных окружения или аргументов см https://github.com/spf13/viper

Comment thread pkg/data/data.go Outdated
import (
"errors"
"fmt"
"github/Rarik88/go_final_project/pkg/model"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

с таким форматирование импортов работу не приму

см go imports

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

нужно чтобы импорты были верно сгруппированы

см go imports

@Rarik88
Copy link
Copy Markdown
Owner Author

Rarik88 commented May 1, 2024 via email

Copy link
Copy Markdown

@dirtymew dirtymew left a comment

Choose a reason for hiding this comment

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

Привет Алекс

дал комментарии что исправить

Comment thread tests/addtask_4_test.go Outdated
"bytes"
"encoding/json"
"fmt"
"github.com/stretchr/testify/assert"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@Rarik88 тут не поправил

Comment thread pkg/data/data.go Outdated
import (
"errors"
"fmt"
"github/Rarik88/go_final_project/pkg/model"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

нужно чтобы импорты были верно сгруппированы

см go imports

Comment thread .dockerignore
**/.project
**/.settings
**/.toolstarget
**/.vs
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

все еще в ветке @Rarik88

Comment thread cmd/main.go
serv := new(app.Server)

err = serv.Run("7540", handler.Init())
if err != nil {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

вот тут было лог что приложение запустилось на таком-то порту @Rarik88

Comment thread pkg/hub/db_sqlite.go Outdated
return &TaskSQLite{db: db}
}

func Sqlite(dbname string) *sqlx.DB {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

тут ошибку похоже забыл вернуть - main ругается @Rarik88

Comment thread pkg/const/const.go Outdated
ENV_PORT = "TODO_PORT" // Переменная окружения для порта
API_NEXTDATE = "/api/nextdate" // URL-путь для API
API_TASK = "/api/task" // URL-путь для API
NOW = "now" // Константа для текущего времени
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

что такое NOW?

и еще куча констант которые не используются - не используешь удаляй

в go не принято не используемые обьекты хранить

Copy link
Copy Markdown

@dirtymew dirtymew left a comment

Choose a reason for hiding this comment

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

Привет Алекс

Поздравляю - задание выполнено

Удачи в дальшейшем обучении

@Rarik88
Copy link
Copy Markdown
Owner Author

Rarik88 commented May 5, 2024 via email

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