search

jjw24 / Wox

Launcher for Windows, an alternative to Alfred and Launchy.
http://wox.one
MIT License
154 stars 12 forks source link

Mydev/dev #57

Closed theClueless closed 5 years ago

theClueless commented 5 years ago

added a conditional compilation symbol called SDK to program plugin in order to give the ability not to install windows SDK (and put it at the csproj). added a public constructor for Query to have the ability to unit test a plugin

jjw24 commented 5 years ago

Thanks for the PR.

With the SDK const, does that mean when compiling for release the whole UWP functionality will not be present in Wox if the dev doesn't have the SDK installed?

Could we seperate the SDK change into a different PR so we can better manage it, and easier + clearer to merge your other changes?

theClueless commented 5 years ago

I also changed the SDK const to IGNORE_UWP const and you need to put it in the CSPROJ now in order to ignore so you won't have issues with release configuration. about splitting the PR to 2 PR's, to be honest I still don't know enough about git in order to fully understand how to do it, do you mind if it is 1 PR?

jjw24 commented 5 years ago

How's it going.

Splitting this in to 2 PRs is pretty straight forward to be honest, it is literally creating a separate branch from your master/mydev branch and copy and paste all the changes from this pr into the new branch and discard the changes that you don't need.

Since this PR's branch is behind Dev already, you will need to merge the latest in and resolve conflicts, so might as well separate these changes if you could.

jjw24 commented 5 years ago

All in all I like the disabling/excluding UWP SDK proposal, this will allow other contributors to quickly fire up this project, make changes or debug without having to download that big SDK.

We need a better approach from your proposed changes in this PR though, because throwing all those if statements in there is not pretty, future contributors may accidentally forget to if out their new UWP commands.

A maybe approach could be refactoring them into their own method class so you only need to do one if to exlcude, and exclude only during debug mode if the developer doesn't have SDK installed, and will throw exception if trying to release build.

All above are just hacky workarounds, ultimate solution is move away from the sdk but that's a whole different scope of work.

Maybe for now keep the SDK changes separate to this PR(*apologies) so more discussions and thinking can be put into it.

theClueless commented 5 years ago

split hence closing the current PR