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

Adding Windows Services, Registry Core Ext from PT Run #120

Merged
merged 5 commits into from
Oct 31, 2024

Conversation

joadoumie
Copy link
Collaborator

First pass at adding the equivalent functionality of Windows Services from PT Run.

@joadoumie
Copy link
Collaborator Author

This PR includes first passes for both the Windows Services Plugin & the Windows Registry Plugin

@joadoumie joadoumie requested a review from zadjii-msft October 25, 2024 17:22
Copy link
Owner

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

okay I reviewed the first two commits - services and registry

  • I didn't look too hard at the implementation bits that are clearly straight outta PT run. I assume that code's already fine.
  • I left a bunch of notes on linking bare TODOs - we're in the real game now (not just hackathon), so we should have TODO's in the code reference an actual work item tracking that TODO. Otherwise we just lose them. In terminal we've used TODO GH #xyz: <comment>to help keep track of the issue that's tracking that TODO
  • I don't know about the resources thing so clint halp pls
    • I'm blocking on this
  • we probably want to actually implement the copy thing now
    • i'm soft-blocking on this
  • revert the settings search one and the commit after that, and file a stacked PR for settings search 😜

@@ -0,0 +1,333 @@
//------------------------------------------------------------------------------
Copy link
Owner

Choose a reason for hiding this comment

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

@crutkas I need a C# adult - this looks like the kind of file that should be .gitignored?


public override ISection[] GetItems(string query)
{
ListItem[] items = ServiceHelper.Search(query).ToArray();
Copy link
Owner

Choose a reason for hiding this comment

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

📝 we could totally cache this result in the future, but good enough for v1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmmmm good point - let's just do it? It's probably best practice and we can use that as a sample down the line?

HOW should we do it the BEST way (best being best for now that is reasonable, but does some caching)

@joadoumie joadoumie force-pushed the joadoumie/windows-services branch from e927e7d to e5c95ee Compare October 29, 2024 13:49
@joadoumie joadoumie requested a review from zadjii-msft October 29, 2024 15:37
@zadjii-msft zadjii-msft changed the title Adding Windows Services Core Ext Adding Windows Services, Registry Core Ext from PT Run Oct 30, 2024
Copy link
Owner

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

@crutkas are these Resources.Designer.cs files supposed to be checked in?

src/modules/cmdpal/Exts/Microsoft.CmdPal.Ext.Registry/Properties/Resources.Designer.cs

Just wanna make sure. Otherwise, this LGTM

Comment on lines 43 to 47
private static bool TryToCopyToClipBoard(in string text)
{
// TODO: Have this actually use the clipboard helper
return true;
}
Copy link
Owner

Choose a reason for hiding this comment

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

dead code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed. Good catch. thank you. I DONT know how this is still in here tbh.

@crutkas
Copy link
Collaborator

crutkas commented Oct 31, 2024

Yes the are included

@zadjii-msft zadjii-msft merged commit 580fe99 into main Oct 31, 2024
@zadjii-msft zadjii-msft deleted the joadoumie/windows-services branch October 31, 2024 15:56
@joadoumie
Copy link
Collaborator Author

Ref to #84

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.

3 participants