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

Added AttachedCommand. #34

Merged
merged 1 commit into from
Apr 5, 2019

Conversation

konrad-kruczynski
Copy link

AttachedCommand is the Command used when attaching to already running
process.

@madelson
Copy link
Owner

Thanks @konrad-kruczynski this looks great! Let me know when you've had a chance to take a look at the comments.

@madelson
Copy link
Owner

This will resolve #30

Copy link
Owner

@madelson madelson left a comment

Choose a reason for hiding this comment

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

why not use HasExited instead of WaitForExit(0)

Good call! HasExited is probably better.

MedallionShell/AttachedCommand.cs Show resolved Hide resolved
MedallionShell/AttachedCommand.cs Outdated Show resolved Hide resolved
MedallionShell/AttachedCommand.cs Show resolved Hide resolved
MedallionShell/Command.cs Outdated Show resolved Hide resolved
MedallionShell/ProcessHelper.cs Show resolved Hide resolved
MedallionShell/Shell.cs Outdated Show resolved Hide resolved
MedallionShell/Shell.cs Show resolved Hide resolved
MedallionShell/Shell.cs Outdated Show resolved Hide resolved
MedallionShell/Shell.cs Show resolved Hide resolved
MedallionShell.Tests/AttachingTests.cs Show resolved Hide resolved
@konrad-kruczynski
Copy link
Author

I've just pushed changes. They are now there as a separate commit, I can squash it later, but I think it might be easier for you to review changes this way. Also please take a look at yet unresolved comments.

I know that I only have "spikes" of activity from time to time, but actually that's all I can do considering available time slots ;)

Copy link
Owner

@madelson madelson left a comment

Choose a reason for hiding this comment

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

Thanks! I think I've gone through and resolved/updated all the comment threads (I'm not super familiar with the GitHub PR UI so if I missed something just post me a link).

I know that I only have "spikes" of activity from time to time, but actually that's all I can do considering available time slots ;)

Don't worry about it! I really appreciate your help and engagement on this!

MedallionShell/AttachedCommand.cs Show resolved Hide resolved
MedallionShell/Shell.cs Outdated Show resolved Hide resolved
MedallionShell/AttachedCommand.cs Outdated Show resolved Hide resolved
@konrad-kruczynski
Copy link
Author

Frankly speaking I'm not accustomed to the Github PR UI (yet) as well, using it for the second time.

Fixed one thing and one is waiting for the answer.

@madelson
Copy link
Owner

madelson commented Apr 3, 2019

Ok so I think everything is resolved now? Are you comfortable with me merging?

@konrad-kruczynski
Copy link
Author

Ok so I think everything is resolved now?

Yes, at least it seems so :)

Are you comfortable with me merging?

Yes, I am. Should I squash the commits into one?

@madelson
Copy link
Owner

madelson commented Apr 5, 2019

Yes, I am. Should I squash the commits into one?

If you get a chance to do in the next 24 hours or so then go for it. If not, I'll probably just go ahead and merge; I'm not a purist when it comes to git history

AttachedCommand is the Command used when attaching to already running
process.
@konrad-kruczynski
Copy link
Author

Done.

@madelson madelson merged commit dee56f3 into madelson:release-1.6 Apr 5, 2019
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