Feature/v3.x.x/1022 and 1021 app version process info #85
Conversation
| timeCreated: 1536066842 | ||
| licenseType: Free | ||
| DefaultImporter: | ||
| userData: |
There was a problem hiding this comment.
Git reports this change as moving file, instead of changing it's content.
|
|
||
| public AppVersion GetAppVersionInfo(int versionId) | ||
| { | ||
| if (versionId < 0) |
There was a problem hiding this comment.
As I remember, version id cannot be zero. @genail am I right?
There was a problem hiding this comment.
Most probably you're right. I can't recall any scenario where version is 0. When a version is not published, it just returns 404.
| { | ||
| if (!string.IsNullOrEmpty(appVersion.MainExecutable)) | ||
| { | ||
| return Path.Combine(_app.LocalDirectory.Path, appVersion.MainExecutable); |
There was a problem hiding this comment.
Maybe check if that file exist before returning it as valid executable path?
That would be much more safe.
There was a problem hiding this comment.
I agree. Then report this incident to sentry and fallback to executable search.
Checking if API provided executable exists, otherwise reporting an exception to Sentry and falling back on executable search. A valid version id should be larger than 0.
|
|
||
| // Reports to Sentry | ||
| DebugLogger.LogException( | ||
| new FileNotFoundException(string.Format("Couldn't resolve executable in {0}", executablePath))); |
There was a problem hiding this comment.
If you create an exception you should always throw it. Otherwise stack trace information is not filled inside it.
So better solution for that would be to wrap this code inside try and report exception to Sentry in catch.
witcher112
deleted the
feature/v3.x.x/1022-and-1021-app-version-process-info
branch
No description provided.