Skip to content

Merge definitions of Disposable where possible#2628

Merged
robertbrignull merged 1 commit intomainfrom
robertbrignull/disposable
Jul 24, 2023
Merged

Merge definitions of Disposable where possible#2628
robertbrignull merged 1 commit intomainfrom
robertbrignull/disposable

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

We had three identical versions of Disposable types, all with the intention of avoiding a dependency on VS Code. I propose we can merge them all and just use the one from disposable-objects.ts. That file is already in common and has a comment at the top saying to avoid dependencies on VS Code.

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.

@robertbrignull robertbrignull requested a review from a team July 21, 2023 15:44
@robertbrignull robertbrignull requested a review from a team as a code owner July 21, 2023 15:44
aeisenberg
aeisenberg previously approved these changes Jul 21, 2023
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the intention with packages/commands is that it's a stand-alone package that we could potentially publish to npm one day. So I think we need to leave this implementation here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll revert that one.

It would be better for the future if this requirement were enforced or documented in the code, so we don't have to rely on remembering it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep, agreed!

@aeisenberg aeisenberg dismissed their stale review July 21, 2023 15:58

Revoking my approval since @charisk brings up a point I hadn't considered.

@robertbrignull robertbrignull changed the title Merge all our Disposable definitions Merge definitions of Disposable where possible Jul 21, 2023
@robertbrignull robertbrignull force-pushed the robertbrignull/disposable branch from ae47014 to f1a9289 Compare July 21, 2023 16:07
@robertbrignull
Copy link
Copy Markdown
Contributor Author

PR is updated to just remove the instance from log-scanner.ts

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.

Thank you for tidying that up!

@robertbrignull robertbrignull enabled auto-merge July 21, 2023 16:11
@robertbrignull robertbrignull merged commit 44b5828 into main Jul 24, 2023
@robertbrignull robertbrignull deleted the robertbrignull/disposable branch July 24, 2023 10:20
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