-
Notifications
You must be signed in to change notification settings - Fork 412
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
Add YAML and JSON encoding functions #20
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Отличная работа, но есть несколько моментов. В следующих работах обяхательно создавайте собственный репозиторий с проектом или делайте fork проекта яндекса
По работе, есть ошибка которую нужно исправить, возврат ошибки, все остальное ркоммендации
return nil | ||
} | ||
|
||
err = json.Unmarshal(jsonFile, &j.DockerCompose) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Отлично, что сразу десериализовали в нужную структуру
encoding/encoding.go
Outdated
|
||
yamlFile, err := os.Open(j.FileOutput) | ||
if err != nil { | ||
fmt.Printf("ошибка при открытии файла %s: %s", j.FileOutput, err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Круто, что продумали некоторые вербоус действий, которые происходят в системе
} | ||
} | ||
|
||
defer yamlFile.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Отлично, предусмотрели закрытие файлов после завершения функции
encoding/encoding.go
Outdated
jsonFile, err := os.ReadFile(j.FileInput) | ||
if err != nil { | ||
fmt.Printf("ошибка при чтении %s файла: %s", j.FileInput, err.Error()) | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вот тут есть момент, который нужно исправить во всех похожих случаях. Если мы обрабатываем ошибку, нужно ее вернуть return err
Все, что ниже просто дополнительная информация.
Не могу точно сказать прошли вы этот материал или еще нет, но если вы хотите добавить некоторый контекст в вашу ошибку, то лучше сделать так
return fmt.Errorf("ошибка открытия файла %s: %w", j.FileInput, err)
Тогда мы возвращаем ошибку, но добавляем в нее дополнительный контекст подставляя вместо глагола %w нашу ошибку. И уже этот текст ошибки залогировать наверху в main() где мы получим ошибку с более емким содержимым
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Исправлено
По этому вопросу. Я же вроде и сделал Fork репозитория яндекса, как написано в задании. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хорошо, что исправили, если будет время прочитайте подробнее про механизм обработки ошибок в Go и errorf
Но к сожалению нужно исправить маленькую историю с os.Create, прочитайте коммент
|
||
_, err = yamlFile.Write(yamlData) | ||
if err != nil { | ||
return fmt.Errorf("ошибка при записи данных в файл: %s: %w\n", j.FileOutput, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да, все круто, но нам нужен тут \n
В данном случае fmt.Errorf() возвращает ошибку. Когда вы ее где нибудь наверху поймаете вы сможете отформатировать текст как захотите, поэтому \n лучше убрать
** Правка не обязательная
encoding/encoding.go
Outdated
if err != nil { | ||
fmt.Printf("ошибка при открытии файла %s: %s\n", y.FileOutput, err.Error()) | ||
fmt.Printf("создаем файл %s\n", y.FileOutput) | ||
jsonFile, err = os.Create(y.FileOutput) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Смотрите, в этой задаче какая у нас логика идет
- Сначала в программе ниже создаются входные файлы
- Потом мы читаем файл и десериализуем в структуру
- Потом мы эту структуру сериализуем в новый файл, который нужно создать
В данном случае нам не нужно открывать выходной файл, которого еще нет, нам нужно сразу его создать и записать данные.
То есть os.Open тут лишнее, сразу os.Create тогда и ошибку вы эту не поймаете. Сейчас она у вас видимо возникает, потому что файла нет, а вы пытаетесь его открыть.
Итого: нужно убрать os.Open и оставить только os.Create
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Исправлено. Сначала подумал что все 4ре файла создаются сразу в начале main(), теперь увидел что там только два исходных
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Отлично поработали Егор! Принято!
No description provided.