Allow script extensions to declare unsafe blocks #1753
Conversation
This change also improves the robustness of the `UseWindowsForms` check, only checking in appropriate locations within the project file and disregarding the case of `true` vs `True`. (The latter matters if someone uses the Visual Studio project editor, as it now uses `True` instead of `true`.) Fixes bonsai-rx#1727
There was a problem hiding this comment.
@PathogenDavid Looks great, I like the XPath solution and I definitely didn't know about the VS shenanigans with true vs True so thanks for calling that to attention.
I suggested a possible approach to refactor the XDocument business, simply because the file was already being opened twice in the current implementation. I had left it like this before pretty much for the same reasons to avoid a painful refactoring, but I guess third time's the charm as they say.
…sions` out into `ScriptExtensionsProjectMetadata`. This better encapsulates this logic to avoid re-reading the `csproj` multiple times while avoiding changes to the external behavior of the interface over the lifetime of `ScriptExtensions` instances.
|
(Moved this out to a comment so it doesn't get lost when I resolve the conversation)
I know the old implementation had this, but is this actually a good thing to do? This basically means we couldn't figure out how to reference the requested assembly, should we throw instead? |
The logic behind it is that the compiler will warn about the missing assembly in context and that is usually enough to understand what is missing. I agree it might be friendlier to stop earlier, in that case we would throw here and catch in the bootstrapper so we can print a friendly message similar to the compiler on this. But I would maybe open a new issue to discuss this specific behaviour so we don't make this PR too big. |
The setting can be toggled by modifying
Extensions.csproj, same as any other C# project.This change also improves the robustness of the
UseWindowsFormscheck since I modified it to use the helper I added for this change.In particular it now only checks in appropriate locations within the project file, and perhaps more importantly it disregards the case of
truevsTrue. At some point* Visual Studio started using the latter when these settings are changed via the UI, so anyone using it would've hadTruetreated as false.(*I assume so at least, I typically just modify my
csprojby hand and I don't have an old version of Visual Studio to check at the moment–I would guess it changed when they redid the project configuration tool.)Opening the whole project file to check a single value makes me slightly itchy, but it's consistent with similar logic in
ScriptExtensionsand I didn't want to get lost in the weeds refactoring.Fixes #1727