Select latest database when creating skeleton query#2330
Conversation
elenatanasoiu
changed the base branch from
main
to
yer-a-single-rooted-workspace-query
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
This will work as is, but I have a couple of small questions to which I'm keen to hear your thoughts. This is my first time reading this code so you'll have to forgive me if I'm missing some context or misunderstanding things.
| if (this.language === undefined) { | ||
| throw new Error("Language is undefined"); | ||
| } | ||
| public async findExistingDatabaseItem( |
There was a problem hiding this comment.
These functions are a little confusing as to whether they should be members of the SkeletonQueryWizard class or not, because we're passing in arguments that are otherwise available on this. It makes the functions slightly harder to implement because you have to know whether there's a language argument available and that you should use that instead of this.language. We don't want to end up using both from the same method.
We could make these methods static so then that removes the chance of accidentally referencing this, or move them out of the class so they're just top-level non-exported methods.
There was a problem hiding this comment.
I've converted these types of methods into static versions.
| if (existingDatabaseItem) { | ||
| await this.databaseManager.setCurrentDatabaseItem(existingDatabaseItem); | ||
| } else { | ||
| await this.downloadDatabase(); |
There was a problem hiding this comment.
This isn't directly related to this PR but I've noticed that we could "easily" (famous last words) make downloadDatabase return the newly-created database item. It seems that downloadGitHubDatabase already returns it, so we'd just need to return it from downloadDatabase. That would remove the need to call findExistingDatabaseItem afterwards and potentially remove a chance of race conditions.
There was a problem hiding this comment.
I also just noticed when reviewing #2328 that downloadDatabase should theoretically already set the new database as the currently selected item. It calls through to databaseArchiveFetcher which calls setCurrentDatabaseItem.
Would be good to test if this is working. If it isn't could it be the same single-workspace issue, so once that is fixed this will also work?
There was a problem hiding this comment.
If it isn't could it be the same single-workspace issue, so once that is fixed this will also work?
Yes, that's currently what's happening. Because we decided to select the database when we open it, we no longer need to repeat the step here.
elenatanasoiu
force-pushed
the
yer-a-single-rooted-workspace-query
branch
from
e40a53c to
48391fb
Compare
elenatanasoiu
force-pushed
the
yer-a-sorted-database-query
branch
from
9377845 to
a9c0925
Compare
elenatanasoiu
force-pushed
the
yer-a-single-rooted-workspace-query
branch
2 times, most recently
from
9707095 to
92cbd35
Compare
elenatanasoiu
force-pushed
the
yer-a-sorted-database-query
branch
4 times, most recently
from
edae4a7 to
e944260
Compare
elenatanasoiu
force-pushed
the
yer-a-single-rooted-workspace-query
branch
from
2a97602 to
93a2b06
Compare
elenatanasoiu
force-pushed
the
yer-a-sorted-database-query
branch
from
e944260 to
1821f6f
Compare
Before selecting an existing database, let's sort them by date. We're opting to exclude databases with errors during the sorting process.
And add tests to check this. I've had to adapt the existing `findExistingDatabaseItem` method so receive params so that I'm better able to send it a language and a list of database items.
So that we make it clear we should be passing state as params instead of reading it off `this`.
elenatanasoiu
force-pushed
the
yer-a-sorted-database-query
branch
from
eef05af to
02e1751
Compare
|
Thanks @robertbrignull ! |
2 participants
The skeleton query process tries to choose an existing database for you instead of downloading a new one.
When deciding which database to select, this is the process it goes through:
When we were designing how to find an existing database, we weren't sorting them by their creation date. This means that we might be selecting an older one, instead of the latest.
So let's sort our databases by
dateAddedbefore attempting to select one.