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

Remove ProcessUtil and bump Gapotchenko.FX version #891

Merged
merged 3 commits into from
Dec 29, 2023

Conversation

eksime
Copy link
Contributor

@eksime eksime commented Jun 2, 2023

Discussion Reference

Breaking change: no

Minor cleanup to remove ProcessUtil and replace it with functionality built in to gapotchenko.fx

@eksime eksime added the cleanup label Jun 2, 2023
@eksime eksime requested a review from dlamkins June 2, 2023 18:04
@sonarcloud
Copy link

sonarcloud bot commented Jun 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Jul 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@@ -219,7 +219,7 @@ public sealed class Gw2InstanceIntegration : ServiceModule<GameIntegrationServic
}

try {
this.CommandLine = newProcess.GetCommandLine();
this.CommandLine = newProcess.ReadArguments();
} catch (Win32Exception e) {
Copy link
Member

Choose a reason for hiding this comment

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

I am curious about what exception gets thrown here if ReadArguments() fails. I'll review the lib to double-check.

Copy link
Member

@dlamkins dlamkins Jul 4, 2023

Choose a reason for hiding this comment

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

Yeah, reviewing the source for Gapotchenko.FX.Diagnostics.Process it looks like there are a couple of possible outcomes. All are ultimately probably equally unrecoverable as far as I can tell. Could we change the catch to just catch on Exception and slightly revise the logger line message?

@@ -219,7 +219,7 @@ public sealed class Gw2InstanceIntegration : ServiceModule<GameIntegrationServic
}

try {
this.CommandLine = newProcess.GetCommandLine();
this.CommandLine = newProcess.ReadArguments();
} catch (Win32Exception e) {
Copy link
Member

@dlamkins dlamkins Jul 4, 2023

Choose a reason for hiding this comment

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

Yeah, reviewing the source for Gapotchenko.FX.Diagnostics.Process it looks like there are a couple of possible outcomes. All are ultimately probably equally unrecoverable as far as I can tell. Could we change the catch to just catch on Exception and slightly revise the logger line message?

@dlamkins
Copy link
Member

dlamkins commented Jul 4, 2023

Sorry, I realize updating the branch from my testing is going to make it annoying to commit any changes. I suppose the online GitHub editor might make that easier, though.

Copy link

sonarcloud bot commented Dec 29, 2023

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@dlamkins dlamkins merged commit e909fb3 into blish-hud:dev Dec 29, 2023
5 checks passed
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.

2 participants