Skip to content

Discipline and course API#18

Open
t3m8ch wants to merge 8 commits intomainfrom
t3m8ch/discipline-and-course-api
Open

Discipline and course API#18
t3m8ch wants to merge 8 commits intomainfrom
t3m8ch/discipline-and-course-api

Conversation

@t3m8ch
Copy link
Copy Markdown

@t3m8ch t3m8ch commented Oct 18, 2024

close #9

@t3m8ch t3m8ch added the feature New feature or request label Oct 18, 2024
@t3m8ch t3m8ch added this to the CRUD milestone Oct 18, 2024
@t3m8ch t3m8ch requested a review from granatam October 19, 2024 10:25
@t3m8ch t3m8ch changed the title WIP Discipline and course API Discipline and course API Oct 19, 2024
Copy link
Copy Markdown
Contributor

@granatam granatam left a comment

Choose a reason for hiding this comment

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

В целом всё нормально, но некоторые вопросы/предложения есть.

(Fix CI by cargo sqlx prepare plz)

Comment thread src/models.rs

#[derive(utoipa::ToSchema, Serialize, Deserialize)]
pub struct Discipline {
pub id: Uuid,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Будут ли проблемы, если вместо того, чтобы плодить удвоенное количество структур, использовать pub id: Option<Uuid>?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(да, это я писал 3 структуры пользователя)

Copy link
Copy Markdown
Author

@t3m8ch t3m8ch Oct 19, 2024

Choose a reason for hiding this comment

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

Тут вопрос философский. Я больше сторонник того, чтобы разделять структуры, которые для апи, и структуры, которые в БД. Даже если в моменте они отличаются минимально, в будущем они могут начать отличаться сильно. С другой стороны, мы же сейчас прототип делаем, поэтому возможно нету смысла дублировать структуры, если они отличаются только одним полем (вот, например, CourseIn я бы не заменял на опциональные поля, там больше одного поля отличия).

Comment thread src/discipline/handlers.rs Outdated
Comment thread src/course/handlers.rs Outdated
Comment thread src/db/course.rs
Comment thread src/discipline/handlers.rs
@t3m8ch t3m8ch requested a review from granatam October 19, 2024 21:02
Copy link
Copy Markdown
Contributor

@granatam granatam left a comment

Choose a reason for hiding this comment

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

Удали лишние логи из handlers/... функций (логгер actix сам справляется в этих местах) и можно либо вливать, либо добавить кого-нибудь еще на ревью.

И желательно сделать rebase в 2 feat коммита (либо в 3 с третьим - фиксом CI, так просто проще будет ребейзить).

Comment thread src/course/handlers.rs
)]
#[get("/courses")]
async fn get_all(ctx: Data<Context>) -> HttpResponse {
log::trace!("Received get courses request");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Вижу логи в db/... функциях, тут они не нужны, тем более если нет лога после let Ok(courses) = ....

Аналогично с другими.

Comment thread src/course/handlers.rs
}

#[utoipa::path(
request_body = CourseIn,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Это хорошо, желательно отдельным коммитом/PR-ом добавить это в auth модуль.

Copy link
Copy Markdown
Contributor

@evgenymng evgenymng left a comment

Choose a reason for hiding this comment

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

После слияния #21 придётся делать rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create discipline & course API

3 participants