Skip to content

Add commands package inside src directory#2148

Merged
koesie10 merged 2 commits intomainfrom
koesie10/commands-package
Mar 10, 2023
Merged

Add commands package inside src directory#2148
koesie10 merged 2 commits intomainfrom
koesie10/commands-package

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Mar 8, 2023

This adds a new packages/commands directory with a stub implementation of a CommandManager, in preparation for implementing this command package.

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.

@koesie10 koesie10 added the secexp label Mar 8, 2023
@koesie10 koesie10 marked this pull request as ready for review March 8, 2023 15:58
@koesie10 koesie10 requested a review from a team as a code owner March 8, 2023 15:58
@koesie10 koesie10 requested a review from a team March 8, 2023 15:58
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Short and sweet :)

describe(CommandManager.name, () => {
it("can create a command manager", () => {
const commandManager = new CommandManager();
expect(commandManager).not.toBeUndefined();
Copy link
Copy Markdown
Contributor

@elenatanasoiu elenatanasoiu Mar 9, 2023

Choose a reason for hiding this comment

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

I agree we want a test file for this, but I would argue this is actually testing Typescript functionality rather than something specific to the CommandManager.

We can probably overwrite it later with some more concrete testing (and maybe that's what you were thinking as well?).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, exactly, this was meant as a test just to have a test, but doesn't actually test anything. When we actually do add tests, we'll remove this test.

@koesie10 koesie10 enabled auto-merge March 10, 2023 09:28
@koesie10 koesie10 merged commit 807069e into main Mar 10, 2023
@koesie10 koesie10 deleted the koesie10/commands-package branch March 10, 2023 09:32
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.

3 participants