Get details for one book #14

Merged
j285he merged 8 commits from j285he-one-book into main 5 months ago
j285he commented 6 months ago
Collaborator

Closes #3

Closes #3
j285he added 2 commits 6 months ago
j285he reviewed 6 months ago
lib/books.ts Outdated
}
});
});
// .then((newBook) => {
Poster
Collaborator

Some questions...

Would it be better to have the database.close in the finally statement? Also, would it be benificial to have the catch statement so that error messages are console.logged?

However, adding the catch statement changes the type of book to be Promise<unknown>. I believe this is because the catch function doesn't return anything, the promise returned by then gets resolved with an undefined value..

Some questions... Would it be better to have the database.close in the `finally` statement? Also, would it be benificial to have the `catch` statement so that error messages are console.logged? However, adding the catch statement changes the type of book to be `Promise<unknown>`. I believe this is because the catch function [doesn't return anything, the promise returned by then gets resolved with an undefined value.](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then).
Owner

Good questions!

Would it be better to have the database.close in the finally statement?

This is a good question! It seems like if we put the database.close in the finally block, then the database is only closed after the promise has been fulfilled or rejected, whereas if the database.close is not in the finally block, it seems like the database closes earlier but everything still works? Maybe @a3thakra or @n3parikh can give some insight...

Also, would it be benificial to have the catch statement so that error messages are console.logged?

This probably isn't necessary, as any errors should be logged automatically.

I also found this article about error handling that has a short section on "Returning errors in promises" - it seems like we can just reject(err), and then when we call getBook from the frontend or wherever we can consider catching errors there.

Good questions! > Would it be better to have the database.close in the `finally` statement? This is a good question! It seems like if we put the `database.close` in the `finally` block, then the database is only closed after the promise has been fulfilled or rejected, whereas if the `database.close` is not in the `finally` block, it seems like the database closes earlier but everything still works? Maybe @a3thakra or @n3parikh can give some insight... > Also, would it be benificial to have the `catch` statement so that error messages are console.logged? This probably isn't necessary, as [any errors should be logged automatically](https://stackoverflow.com/a/50896443). I also found [this article](https://medium.com/swlh/error-handling-in-javascript-a-quick-guide-54b954427e47) about error handling that has a short section on "Returning errors in promises" - it seems like we can just `reject(err)`, and then when we call `getBook` from the frontend or wherever we can consider catching errors there.
j285he requested review from n3parikh 6 months ago
j285he requested review from a258wang 6 months ago
j285he added 1 commit 6 months ago
j285he added 1 commit 6 months ago
0028372789 Merge branch 'main' into j285he-one-book
j285he added 1 commit 6 months ago
955c906f4e Fix type for title
a258wang reviewed 5 months ago
lib/books.ts Outdated
const database = new sqlite3.Database(DATABASE_PATH, sqlite3.OPEN_READONLY);
const sql =
"SELECT isbn, lccn, title, subtitle, authors, edition, publisher, publish_year, publish_month, publish_location, pages, pagination, weight, last_updated, deleted FROM books WHERE id = ? ";
Owner

Do we want to be able to query for deleted books, given the deleted book's id? Or should we perhaps restrict ourselves to only books with deleted = 0?

Do we want to be able to query for deleted books, given the deleted book's id? Or should we perhaps restrict ourselves to only books with `deleted = 0`?
Poster
Collaborator

Good idea, querying a deleted book now throws an error

Good idea, querying a deleted book now throws an error
j285he marked this conversation as resolved
j285he added 2 commits 5 months ago
f1994b2f5d Merge main
e26chiu reviewed 5 months ago
isbn: string | null;
lccn: string | null;
title: string | null;
authors: string | null;
Collaborator

Are the authors going to be stored in a list of strings or are we going to have one string with the list of all author names separated by a comma?

Are the authors going to be stored in a list of strings or are we going to have one string with the list of all author names separated by a comma?
Owner

The database stores the authors field as one string, with names separated by commas.

The database stores the authors field as one string, with names separated by commas.
a258wang reviewed 5 months ago
lib/books.ts Outdated
const database = new sqlite3.Database(DATABASE_PATH, sqlite3.OPEN_READONLY);
const sql =
"SELECT isbn, lccn, title, subtitle, authors, edition, publisher, publish_year, publish_month, publish_location, pages, pagination, weight, last_updated, deleted FROM books WHERE id = ? AND deleted = 0";
Owner

Just a small nitpick, we probably don't need to SELECT deleted FROM books since we know we're only querying for books with deleted = 0.

Just a small nitpick, we probably don't need to `SELECT deleted FROM books` since we know we're only querying for books with `deleted = 0`.
j285he marked this conversation as resolved
j285he added 1 commit 5 months ago
21985f8aa6 Change SQL query
a258wang approved these changes 5 months ago
a258wang left a comment
Owner

👍

👍
j285he merged commit 2040c63311 into main 5 months ago
j285he referenced this issue from a commit 5 months ago

Reviewers

n3parikh was requested for review 6 months ago
a258wang approved these changes 5 months ago
continuous-integration/drone/pr Build is passing
continuous-integration/drone/push Build is passing
The pull request has been merged as 2040c63311.
Sign in to join this conversation.
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.