Skip to content

Repo state store naming#2311

Merged
charisk merged 6 commits intomainfrom
charisk/repo-state-store-naming
Apr 13, 2023
Merged

Repo state store naming#2311
charisk merged 6 commits intomainfrom
charisk/repo-state-store-naming

Conversation

@charisk
Copy link
Copy Markdown
Contributor

@charisk charisk commented Apr 13, 2023

Reviewed names used in the repo states store, models (data/domain) and mappers, and updated them to match conventions agreed.

  • Renamed data types to be referred to as DTOs.
  • Renamed mapping functions to use the DTO term but also to follow the same naming patterns.
  • Renamed and moved files to match convention.
  • Introduced an index file for the repo states store.
  • Introduced mapping functions for the top level models.

This PR looks fairly big but the changes are pretty small and straight forward. Please review commit by commit to see the differences.

Checklist

N/A:

  • 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.

@charisk charisk added the secexp label Apr 13, 2023
@charisk charisk requested a review from a team as a code owner April 13, 2023 09:52
@charisk charisk removed the request for review from RobertBolender April 13, 2023 09:54
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes LGTM

The index.ts file is an approach I haven't seen us use before, but it makes sense to designate repo-states-store.ts as the canonical entry point to that whole directory.

@charisk
Copy link
Copy Markdown
Contributor Author

charisk commented Apr 13, 2023

All changes LGTM

The index.ts file is an approach I haven't seen us use before, but it makes sense to designate repo-states-store.ts as the canonical entry point to that whole directory.

FWIW we've been using it in a few places (e.g. https://github.com/github/vscode-codeql/blob/main/extensions/ql-vscode/src/common/index.ts). Probably something we should talk about standardising though (and making visible!)

@charisk charisk merged commit 9142fed into main Apr 13, 2023
@charisk charisk deleted the charisk/repo-state-store-naming branch April 13, 2023 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants