feat: add deleted to table Doc

[tomasciccola]

would close #8

[tomasciccola]

I intuitively added deleted to the IndexableDocument type on index.js, but tests started to fail.
On docSchemas I also added deleted as a notnull:0 field with a type of INTEGER (since sqlite stores booleans as ints 1 | 0), but there's probably something else that I'm missing.....
@gmaclennan any pointers?

[gmaclennan]

CI tests are failing because of the change to package-lock

The approach sounds right, but I don't think the code you have done is pushed? Tests will need updated of course to include tests for deleted docs. The syntax for an optional prop in JSDoc is @property {true} [deleted]

[tomasciccola]

CI tests are failing because of the change to package-lock

npm ERR! Cannot read property '@types/better-sqlite3' of undefined
is that what this error refers to??

The approach sounds right, but I don't think the code you have done is pushed?

Is there any more to add besides the IndexableDocument type?

Tests will need updated of course to include tests for deleted docs.

For starters, if we add deleted as an optional, wouldn't all test pass?

[gmaclennan]

A few thoughts and pointers:

1. We need to store deleted in the index, because we read data from the index, so it needs to be in the SQLite table
2. Things are going to be easier if we define deleted as a field that always exists, and is either true or false
3. assertMatchingSchema should check that the deleted field is in the schema (because it needs to be there see (1))
4. Tests will need updated to create a table with the correct schema, and any assertions that are not expecting the deleted field will need updated
5. Can you revert the package-lock.json - I can't really evaluate why tests are failing because tests aren't running in CI.

[github-actions]

Pull Request Test Coverage Report for Build 6315587175

• 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 6025406035:	0.0%
Covered Lines:	330
Relevant Lines:	330

---

💛 - Coveralls

[gmaclennan]

This should return deleted as a boolean - tests are expecting a number 0.

[gmaclennan]

Can you include a test that actually tests when deleted=true? Maybe in the scenarios?

[tomasciccola]

I made deleted notnull:1 and a required field on IndexableDocument
I also updated the tests to contemplate that.
The only doubt that I have is that when writting a Doc with deleted: false, when we read it (with .getDoc()) we get deleted: 0, which is the desired behaviour since Sqlite treats booleans as ints (0 | 1).
But, should we cast the returned value to a boolean when we read it back or should we leave it as that?
Also, what tests should we add regarding deleted? I could modify the tests so we sometime test deleted: true values (expecting deleted: 1 when read back), but other than that I dunno...

[gmaclennan]

Great stuff 👍

[gmaclennan]

I made deleted notnull:1 and a required field on IndexableDocument
I also updated the tests to contemplate that.
The only doubt that I have is that when writting a Doc with deleted: false, when we read it (with .getDoc()) we get deleted: 0, which is the desired behaviour since Sqlite treats booleans as ints (0 | 1).
But, should we cast the returned value to a boolean when we read it back or should we leave it as that?
Also, what tests should we add regarding deleted? I could modify the tests so we sometime test deleted: true values (expecting deleted: 1 when read back), but other than that I dunno...

@tomasciccola just wanted to follow up on this so that there is a record of it. Because we read this data via drizzle, that takes care of converting 1 | 0 into a boolean on read. It's the same with the JSON fields (drizzle does JSON parse). We definitely shouldn't cast it because that would be a lie and would lead to bugs. How you have done it is good.

[coveralls]

Pull Request Test Coverage Report for Build 6315587175

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 6025406035:	0.0%
Covered Lines:	330
Relevant Lines:	330

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 6315587175

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 6025406035:	0.0%
Covered Lines:	330
Relevant Lines:	330

---

💛 - Coveralls