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

Iter20: Указание версии приложения при сборке и вывод этой версии при старте приложения. #21

Merged
merged 16 commits into from
Jul 14, 2024

Conversation

kosalnik
Copy link
Owner

@kosalnik kosalnik commented Jul 3, 2024

No description provided.

@kosalnik kosalnik requested a review from far4599 July 4, 2024 08:22
@kosalnik kosalnik requested review from Perederey and removed request for far4599 July 8, 2024 10:29
Copy link
Collaborator

@Perederey Perederey left a comment

Choose a reason for hiding this comment

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

Привет! Ты хорошо поработал над кодом, но есть несколько моментов, которые стоит улучшить:

  • Обрати внимание на именование функций и методов, чтобы они лучше отражали их назначение
  • Улучши обработку ошибок, особенно в местах, где сейчас используется паника
    В целом, ты на правильном пути. Продолжай работать над улучшением проекта

func parseFlags(args []string, c *config.Agent) {
fs := flag.NewFlagSet(args[0], flag.PanicOnError)
fs.SetOutput(os.Stdout)
fs.StringVar(&c.CollectorAddress, "a", "127.0.0.1:8080", "address server endpoint")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Используй константы вместо магических чисел для значений по умолчанию флагов

"github.com/stretchr/testify/require"
)

func Test_parseFlags(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Группируй связанные тестовые случаи в подтесты с помощью t.Run для улучшения структуры и читаемости тестов

fs.BoolVar(&c.Backup.Restore, "r", true, "Restore storage before start")
fs.StringVar(&c.DB.DSN, "d", "", "Database DSN")
fs.StringVar(&c.Hash.Key, "k", "", "SHA256 Key")
if err := fs.Parse(args[1:]); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Используй структурированную обработку ошибок вместо паники при ошибке парсинга флагов

flag.StringVar(&c.DB.DSN, "d", "", "Database DSN")
flag.StringVar(&c.Hash.Key, "k", "", "SHA256 Key")
flag.Parse()
func parseFlags(args []string, c *config.Server) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


if _, err := db.ExecContext(ctx, schemaCounterSQL); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

лучше переименуй метод InitTables в CreateTablesIfNotExist для более точного описания его функции

"github.com/sirupsen/logrus"
)

type Build struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Переименуй структуру Build в VersionInfo для более точного отражения ее содержимого


if _, err := db.ExecContext(ctx, schemaCounterSQL); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

В методе InitTables можно использовать транзакцию для создания таблиц, чтобы обеспечить атомарность операции. Это повысит надежность кода. Пример:

func (d *DBStorage) InitTables(ctx context.Context) error {
    tx, err := d.db.BeginTx(ctx, nil)
    if err != nil {
        return err
    }
    defer tx.Rollback()

    if _, err := tx.ExecContext(ctx, schemaCounterSQL); err != nil {
        return err
    }
    if _, err := tx.ExecContext(ctx, schemaGaugeSQL); err != nil {
        return err
    }

    return tx.Commit()
}

"golang.org/x/tools/go/types/typeutil"
)

var ExitOnMainAnalyzer = &analysis.Analyzer{
Copy link
Collaborator

Choose a reason for hiding this comment

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

тут хорошо!

"github.com/sirupsen/logrus"
)

var Logger = logrus.New()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Рассмотри возможность избавиться от глобальных переменных

И лучше используй zap или zerolog

@kosalnik kosalnik merged commit ee3433f into main Jul 14, 2024
5 checks passed
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.

None yet

2 participants