Repo States: new data model and mapping

[norascheuch]

This PR introduces new data types for storing repo states and uses them to map between data and domain model.

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[norascheuch]

I have two questions for the reviewer:

• should we add to the existing logic and catch an error when reading or writing from file?
• should we create a mapper for the enum as we do here ?

[robertbrignull]

should we add to the existing logic and catch an error when reading or writing from file?

I'm also wondering if we need some more error handling here. Now we're no longer just returning the raw data, but doing some processing here.

I think we can assume that if it does exist then it contains valid data, since we're leaving that concern for when we add JSON schema validation later on. But right now what do we do in case that the JSON file doesn't exist?

I've tried to read the documentation for this method and it's not clear to me how it behaves in cases of errors. My best guess by looking at the error handling the existing code has is that it returns undefined. It would be good to find some docs, or to test that manually and see.

[robertbrignull]

should we create a mapper for the enum

I would say yes, unless you want to do it in 2 PRs. But I think we do want a mapper for this type as well, particularly because of the enum. It should be a lot simpler to add than the query history types, so doing it in this PR shouldn't be infeasible.

[robertbrignull]

A couple of suggestions / corrections, but this is looking very good generally and almost there. We can likely merge this afternoon.

[robertbrignull]

Nice! This looks good. It's good these "data types" files don't import any other files, and that shows we don't accidentally depend on a domain model.

[robertbrignull]

I like moving the catch to be inside readRepoStates. Unfortunately rewriting this changed the behaviour so now this will currently always overwrite the repo states with {}, which isn't what we want.

We could put the case for when repoStatesFromDisk is undefined into an else block. Or we could use || {} to supply this default value.

Suggested change

	if (repoStatesFromDisk) {
	this.repoStates.set(variantAnalysis.id, repoStatesFromDisk);
	}
	this.repoStates.set(variantAnalysis.id, {});
	this.repoStates.set(variantAnalysis.id, repoStatesFromDisk || {})

[robertbrignull]

Suggested change

	return VariantAnalysisScannedRepositoryDownloadData.Pending;
	assertNever(downloadedStatus);

This is another good case for using assertNever. When mapping from domain to data models, we don't have to worry about badly typed data and having to handle every possibility, so we can say this case will never happen and the type system will enforce it.