www
/
library
22
1
Fork 0
Code Issues 6 Pull Requests Projects Releases Wiki Activity

Get details for one book #14

Merged
j285he merged 8 commits from j285he-one-book into main 2022-03-20 22:22:58 -04:00
Conversation 0 Commits 8 Files Changed 3 +56 -11
j285he commented 2022-02-15 22:26:52 -05:00
Member
Copy Link

Closes #3

Closes #3
j285he added 2 commits 2022-02-15 22:26:53 -05:00
j285he reviewed 2022-02-15 22:33:57 -05:00
lib/books.ts Outdated
@ -0,0 +20,4 @@
}
});
});
// .then((newBook) => {
j285he commented 2022-02-15 22:33:31 -05:00
Author
Member
Copy Link

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).
a258wang commented 2022-02-28 11:55:41 -05:00
Owner
Copy Link

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 2022-02-15 23:09:43 -05:00
j285he requested review from a258wang 2022-02-15 23:09:45 -05:00
j285he added 1 commit 2022-02-16 08:55:53 -05:00
j285he added 1 commit 2022-02-17 09:38:32 -05:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is passing Details
0028372789 Merge branch 'main' into j285he-one-book
j285he added 1 commit 2022-02-22 23:08:43 -05:00
continuous-integration/drone/pr Build is passing Details
continuous-integration/drone/push Build is passing Details
955c906f4e Fix type for title
a258wang reviewed 2022-02-28 11:59:31 -05:00
lib/books.ts Outdated
@ -0,0 +6,4 @@
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 = ? ";
a258wang commented 2022-02-28 11:59:31 -05:00
Owner
Copy Link

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`?
j285he commented 2022-03-16 18:13:17 -04:00
Author
Member
Copy Link

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 2022-03-16 18:19:03 -04:00
continuous-integration/drone/pr Build is passing Details
continuous-integration/drone/push Build is passing Details
f1994b2f5d Merge main
e26chiu reviewed 2022-03-18 10:33:19 -04:00
lib/books.ts
@ -25,0 +49,4 @@
isbn: string | null;
lccn: string | null;
title: string | null;
authors: string | null;
e26chiu commented 2022-03-18 10:33:19 -04:00
First-time contributor
Copy Link

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?
a258wang commented 2022-03-18 10:58:08 -04:00
Owner
Copy Link

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 2022-03-18 11:01:10 -04:00
lib/books.ts Outdated
@ -5,0 +6,4 @@
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";
a258wang commented 2022-03-18 11:01:10 -04:00
Owner
Copy Link

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`.
👍 1
j285he marked this conversation as resolved
j285he added 1 commit 2022-03-18 16:42:59 -04:00
continuous-integration/drone/pr Build is passing Details
continuous-integration/drone/push Build is passing Details
21985f8aa6 Change SQL query
a258wang approved these changes 2022-03-20 13:32:06 -04:00
a258wang left a comment
Owner
Copy Link

👍

👍
j285he merged commit 2040c63311 into main 2022-03-20 22:22:58 -04:00
Sign in to join this conversation.
Reviewers
No reviewers
n3parikh
a258wang
Labels
Clear labels   
Backend

   
Frontend
No Label
Backend
Frontend
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: www/library#14
No description provided.
Delete Branch "j285he-one-book"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?