Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: move command_client folder into core #1448

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fidgetingbits
Copy link
Collaborator

This is mostly just a nit, but every time I go to find command_client/ in community I go to the wrong place, because I don't associate it with vscode. Since there's other users of it now, like neovim-talon now, it probably makes sense for this to just become part of core/. It could also be suited to plugin/, but if we ever decided to move all of the app-specific stuff (only vscode and visual_studio atm) into the associated apps/<foo>/ folders then this would be apps/ relying on plugin/ which we don't want.

Copy link
Collaborator

@pokey pokey left a comment

Choose a reason for hiding this comment

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

I agree in principle, but we'll need to figure out how it interacts with the git subtree push. I'll need to do a bit of googling / experimentation to make sure it will work. See the README in this directory if you're not sure what I'm referring to

I'm going to mark this as needs changes for now just so we don't merge, and we'll also need to update the README with the new subtree push command once we figure it out

@pokey
Copy link
Collaborator

pokey commented May 31, 2024

this would be apps/ relying on plugin/ which we don't want.

Hmm not sure bout that one. I def think core shouldn't rely on apps or plugins, but I'm not sure I see an issue with apps relying on plugins

@fidgetingbits
Copy link
Collaborator Author

this would be apps/ relying on plugin/ which we don't want.

Hmm not sure bout that one. I def think core shouldn't rely on apps or plugins, but I'm not sure I see an issue with apps relying on plugins

Ya that makes sense actually. In that case I'd probably opt for it to go in plugin? but really I'm good with either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants