-
Notifications
You must be signed in to change notification settings - Fork 3
fixed-conflicted:Course.ts,Reviews.ts,User.ts #864
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
base: mvp1-dev
Are you sure you want to change the base?
Conversation
|
| ,sortOrder,limit | ||
| } = c.req.valid("query"); | ||
| const program = Effect.gen(function* (){ | ||
| const userCourses = yield* Effect.tryPromise({ |
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.
Use PrismaService from Effect instead of manually handle the error
| const program = Effect.gen(function* (){ | ||
| const userCourses = yield* Effect.tryPromise({ | ||
| try: ()=> | ||
| prisma.cart.findMany({ |
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.
use "select" to select only necessary fields to reduce network latency between database and backend service, speed up database response, reduce backend service memory usage.
| const {studyProgram,academicYear,semester, | ||
| q,genEdType,faculty,day ,timeStart, timeEnd, noPrereq, fitCardId , assessment,sortBy | ||
| ,sortOrder,limit | ||
| } = c.req.valid("query"); |
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.
This line is not formatted. So, I assume that other places too. Please add format on save to this repository.
| } : undefined, | ||
| take: limit ? Number(limit) : undefined, // Useful if you also want to apply the limit | ||
| }), | ||
| catch: (error) => new Error(`Prisma Error: ${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.
Do not write the code as "catch-all-error". Effect.ts will be useless. Try to utilize what Effect.ts introduce (Recommended to deeply read Effect.ts, especially, Error Handling https://effect.website/docs/error-management/expected-errors/)
| q,genEdType,faculty,day ,timeStart, timeEnd, noPrereq, fitCardId , assessment,sortBy | ||
| ,sortOrder,limit | ||
| } = c.req.valid("query"); | ||
| const program = Effect.gen(function* (){ |
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.
Do not call prisma on Controller.
Controller should responsible only for
- Parse request input (e.g. query, body, form data)
- Validate request (e.g. validate input, validate header, authenticate / authorized request)
- Response the request (as text format, json, format, etc.)
For business logic like calculating something, call external data source (e.g. database), it should be from service or repository.
Related comment #843 (comment)
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.
Encourage to add module name as a prefix for each Schema. This will help other find schemas easier.
imagine, if you need to import something in review module. You just need to write "Review" and editor will suggest the rest like.
ReviewCreateBodyRequestSchema
ReviewUpdateParamsRequestSchema
ReviewUpdateBodyRequestSchema
// ...
| created_at: z.string().datetime, | ||
| updated_at:z.string().datetime, |
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.
| created_at: z.string().datetime, | |
| updated_at:z.string().datetime, | |
| created_at: z.iso.datetime(), | |
| updated_at:z.iso.datetime(), |
| id: z.string(), | ||
| email: z.string().email, | ||
| name: z.string(), | ||
| google_id: z.string(), |
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.
inconsistant naming. Please change to PascalCase
| semester: z.string(), | ||
| rating: z.number().int().min(1).max(5), // Assumes a 1-5 star scale | ||
| content: z.string(), | ||
| status: z.enum(["approved", "pending", "rejected"]), // Using enum for specific statuses |
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.
inconsistent enum, please change to UPPERCASE
| "@hono/zod-openapi": "^1.2.0", | ||
| "@hono/zod-validator": "^0.7.6", |
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.
This 2 library is no need for modern Hono app. Please consider remove it.
Why did you create this PR
What did you do
Demo
https://dev.cugetreg.com
Checklist