Skip to content
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

Replace silent failures with explicit failures in index-level.ts #633

Open
thehenrytsai opened this issue Nov 29, 2023 · 4 comments
Open
Labels
good first issue Good for newcomers refactoring Code refactoring with no functional impact

Comments

@thehenrytsai
Copy link
Member

In index-level.ts, there are at least a couple of places where we accept invalid input and either return void/empty array instead of throwing explicit errors.

It is probably better to throw instead so that the caller does not assume the method succeeds with no results:

  1. queryWithInMemoryPaging()
      // if a cursor is provided but we cannot find it, we return an empty result set
      return [];
  1. getStartingKeyForCursor()
    if (indexes === undefined) {
      // invalid itemId
      return;
    }

    const sortValue = indexes[property];
    if (sortValue === undefined) {
      // invalid sort property
      return;
    }
@thehenrytsai thehenrytsai added good first issue Good for newcomers refactoring Code refactoring with no functional impact labels Nov 29, 2023
@lchauha
Copy link

lchauha commented Nov 30, 2023

Hi @thehenrytsai I would like to work on this issue. Can you please assign this to me?

@lchauha
Copy link

lchauha commented Nov 30, 2023

The two methods that you have specified above, I can't find them in index-level.ts file. Were these (queryWithInMemoryPaging(), getStartingKeyForCursor() ) examples?

@lchauha
Copy link

lchauha commented Nov 30, 2023

I have just found one place where we are not returning anything in the index-level.ts file.

https://github.com/TBD54566975/dwn-sdk-js/blob/main/src/store/index-level.ts#L157C14-L157C14, this is the only place.

Let me know if there are more of these.

@LiranCohen
Copy link
Member

Hi @lchauha! This is an Issue related to an existing PR that's being merged in. We preemptively created this Issue to address some things in a separate PR.

Keep an eye out for this PR #625 to be merged and then you will be able to pick this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers refactoring Code refactoring with no functional impact
Projects
None yet
Development

No branches or pull requests

3 participants