Fix (some) build warnings #1314

nachmore · 2022-08-08T04:48:22Z

Primarily fixing a bunch of:

CS1572
CS1573
CS0168
CS8073
CA2200
VSTHRD110
VSTHRD200
VSTHRD105
SYSLIB0013
CS8524

Not a comprehensive fix, but removes a large number of warnings.

CS1572 CS1573 CS0168 CS8073 CA2200 VSTHRD110 VSTHRD200 VSTHRD105 SYSLIB0013 CS8524

…into warnings

While it reflects reality it may prevent some launches on systems that are happily using Flow without hitting specific APIs.

nachmore · 2022-08-08T04:55:00Z

Before: 400 warnings
After: 161 warnings

taooceros

It's a non-trivial refactor. Thank you so much for taking time to do it!

.editorconfig

taooceros · 2022-08-08T16:35:27Z

Flow.Launcher.Core/Configuration/Portable.cs

@@ -127,7 +127,7 @@ public void CreateUninstallerEntry()

            using (var portabilityUpdater = NewUpdateManager())
            {
-                portabilityUpdater.CreateUninstallerRegistryEntry();
+                _ = portabilityUpdater.CreateUninstallerRegistryEntry();


I think we should make CreateUninstallerEntry either async Task or async void. I am curious what this method is doing.

async void needs to be avoided, in fact I want to clean up some of those in the next step if I get a chance.

For this one, it just so happens that Squirrel's function is async, but we're not bothering to call it asynchronously. In fact, nothing in IPortable.cs seems to be async.

I'm happy to create a follow up task for this, but changing the signature of the interface seems like a more invasive task than just removing the warning from the existing flow.

if we didn't await the task, any exception thrown won't be caught. I wonder whether that may create some issue (previously I have been deeply hurt by this suppression).

Flow.Launcher.Core/Plugin/PluginsLoader.cs

Flow.Launcher/HotkeyControl.xaml.cs

Plugins/Flow.Launcher.Plugin.Explorer/Search/SearchManager.cs

Plugins/Flow.Launcher.Plugin.Shell/Main.cs

Plugins/Flow.Launcher.Plugin.WebSearch/SuggestionSources/Baidu.cs

taooceros · 2022-08-08T23:20:05Z

@nachmore One of the build test has failed. Would you please take a look on what's causing that?

nachmore · 2022-08-08T23:21:33Z

I'm looking at this now - it's due to a change in behaviour handling null in json. So much for "just replace X with Y" 😜

Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs

See comment inline re:JSON null

nachmore · 2022-08-09T00:42:29Z

Looking at build mismatch...

Intellisense for the non-win

nachmore · 2022-08-09T00:55:52Z

I'm going to pause the warnings elimination work as this PR is large enough already. So when folks have time, can you please review these changes, I'll address comments and then continue in a separate PR. Thanks!

Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs

Flow.Launcher.Core/Plugin/PluginsLoader.cs

Flow.Launcher.Plugin/AllowedLanguage.cs

Flow.Launcher/Helper/SingleInstance.cs

Plugins/Flow.Launcher.Plugin.Program/Programs/UWP.cs

taooceros · 2022-08-09T01:11:52Z

Plugins/Flow.Launcher.Plugin.Program/Programs/UWP.cs

@@ -540,7 +539,7 @@ internal string LogoUriFromManifest(AppxPackageHelper.IAppxManifestApplication a
                if (logoKeyFromVersion.ContainsKey(Package.Version))
                {
                    var key = logoKeyFromVersion[Package.Version];
-                    app.GetStringValue(key, out string logoUri);
+                    _ = app.GetStringValue(key, out string logoUri);


same as above

nachmore · 2022-08-10T03:27:36Z

Down to 118.

Could I please get a review of these changes? I created follow up tasks for some of the fundamental comments that were uncovered during this PR (but not directly related to these changes).

taooceros · 2022-08-10T03:44:38Z

Down to 118.

Could I please get a review of these changes? I created follow up tasks for some of the fundamental comments that were uncovered during this PR (but not directly related to these changes).

Sure, just give me sometime.

taooceros

Thank you so much for making such a huge change. There's a small pieces that worth taking a look before merging. After addressing them, I think we shall be able to merge.

Let's wrap this with Task.Run and make the method Async

Flow.Launcher/Plugins/Flow.Launcher.Plugin.Program/Programs/UWP.cs

Line 421 in 30669e8

Main.StartProcess(Process.Start, info);

Flow.Launcher/HotkeyControl.xaml.cs

Plugins/Flow.Launcher.Plugin.Program/Programs/UWP.cs

nachmore · 2022-08-10T18:45:51Z

Sure, just give me sometime.

Sorry if it sounded like I was rushing you - I am blown away by the speed of CRs in this project, it's incredible! Thank you for the super quick support...

taooceros · 2022-08-10T18:50:27Z

Sure, just give me sometime.

Sorry if it sounded like I was rushing you - I am blown away by the speed of CRs in this project, it's incredible! Thank you for the super quick support...

Na you are good. It's always easier for me to review than you writing the code, and it will be super nice to hear feedback quickly when contributing.
Let's finish the remaining trivial stuff and merge this one.

taooceros · 2022-08-11T14:41:23Z

Thank you so much for making such a huge change. There's a small pieces that worth taking a look before merging. After addressing them, I think we shall be able to merge.

Let's wrap this with Task.Run and make the method Async

Flow.Launcher/Plugins/Flow.Launcher.Plugin.Program/Programs/UWP.cs

Line 421 in 30669e8

Main.StartProcess(Process.Start, info);

I think this may not be useful and we are ready to merge.

nachmore added 3 commits August 7, 2022 21:31

Fix (some) build warnings

a3197f8

CS1572 CS1573 CS0168 CS8073 CA2200 VSTHRD110 VSTHRD200 VSTHRD105 SYSLIB0013 CS8524

Merge branch 'warnings' of https://github.com/nachmore/Flow.Launcher …

eb26070

…into warnings

Remove explicit platform from AssemblyInfo

4a05a32

While it reflects reality it may prevent some launches on systems that are happily using Flow without hitting specific APIs.

nachmore changed the title ~~Fix (some) warnings~~ Fix (some) build warnings Aug 8, 2022

taooceros requested changes Aug 8, 2022

View reviewed changes

nachmore added 2 commits August 8, 2022 11:27

Addressing initial feedback

ffd8772

Addressing more warnings

b853991

taooceros reviewed Aug 8, 2022

View reviewed changes

Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs Outdated Show resolved Hide resolved

Revert JSON Null change + fix more warnings

3648126

See comment inline re:JSON null

Remove spuriously added discard (_=) for void call

b38ffef

Intellisense for the non-win

taooceros reviewed Aug 9, 2022

View reviewed changes

This was referenced Aug 9, 2022

Move Notification to Infrastructure so that it is useable across Flow #1318

Open

Investigate null serialization on JsonRPCPlugin's JsonSerializerOptions options #1319

Open

nachmore added 4 commits August 9, 2022 16:31

Clean up exception throwing code for plugin loading

dbe2151

AllowedLanguage documentation

03cedcc

Remove unused code

13c70a9

Further cleanups

30669e8

taooceros requested changes Aug 10, 2022

View reviewed changes

Flow.Launcher/HotkeyControl.xaml.cs Outdated Show resolved Hide resolved

Plugins/Flow.Launcher.Plugin.Program/Programs/UWP.cs Outdated Show resolved Hide resolved

nachmore added 2 commits August 10, 2022 11:52

Combine string

719fc99

return Task directly

4a1c585

taooceros approved these changes Aug 11, 2022

View reviewed changes

taooceros merged commit af7efd8 into Flow-Launcher:dev Aug 11, 2022

jjw24 assigned nachmore Aug 11, 2022

jjw24 added the bug Something isn't working label Aug 11, 2022

jjw24 added this to the 1.10.0 milestone Aug 11, 2022

nachmore deleted the warnings branch August 11, 2022 21:12

jjw24 mentioned this pull request Dec 12, 2022

Release 1.10.0 | Plugin 3.0.0 #957

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix (some) build warnings #1314

Fix (some) build warnings #1314

nachmore commented Aug 8, 2022

nachmore commented Aug 8, 2022

taooceros left a comment

taooceros Aug 8, 2022

nachmore Aug 8, 2022

taooceros Aug 8, 2022

taooceros commented Aug 8, 2022

nachmore commented Aug 8, 2022

nachmore commented Aug 9, 2022

nachmore commented Aug 9, 2022

taooceros Aug 9, 2022

nachmore commented Aug 10, 2022

taooceros commented Aug 10, 2022

taooceros left a comment

nachmore commented Aug 10, 2022

taooceros commented Aug 10, 2022

taooceros commented Aug 11, 2022

Fix (some) build warnings #1314

Fix (some) build warnings #1314

Conversation

nachmore commented Aug 8, 2022

nachmore commented Aug 8, 2022

taooceros left a comment

Choose a reason for hiding this comment

taooceros Aug 8, 2022

Choose a reason for hiding this comment

nachmore Aug 8, 2022

Choose a reason for hiding this comment

taooceros Aug 8, 2022

Choose a reason for hiding this comment

taooceros commented Aug 8, 2022

nachmore commented Aug 8, 2022

nachmore commented Aug 9, 2022

nachmore commented Aug 9, 2022

taooceros Aug 9, 2022

Choose a reason for hiding this comment

nachmore commented Aug 10, 2022

taooceros commented Aug 10, 2022

taooceros left a comment

Choose a reason for hiding this comment

nachmore commented Aug 10, 2022

taooceros commented Aug 10, 2022

taooceros commented Aug 11, 2022