ndmitchell / nsis

Haskell DSL for producing Windows Installer using NSIS
Other
26 stars 8 forks source link

Add unsafeInject and unsafeInjectGlobal #11

Closed jmitchell closed 7 years ago

jmitchell commented 7 years ago

Escape hatch so users can inject arbitrary strings into the nsi script. Useful in cases where the DSL doesn't yet support an NSIS feature, e.g. the new Unicode option (see #10).

This is a potential solution to issue #7.

domenkozar commented 7 years ago

cc @ndmitchell

ndmitchell commented 7 years ago

I think this is a great idea - more of the library should be implemented as sugar over injected stuff, without everything gaining an entry in the AST. My concerns are:

jmitchell commented 7 years ago

Are you able to inject arbitrary stuff?

Yes, both functions inject strings directly. I like the names you suggested, but maybe they should be called unsafeInject and unsafeInjectGlobal to stress the risk.

(And if that [optimisation compatibility] is an issue is just turning off optimisations more sensible - how valuable are they in actuality...)

I wouldn't want to do away with the optimizations. When I need to troubleshoot the generated script it's nice that it's so simplified and direct. Adding comments to these functions suggesting to use nsisNoOptimise when unexpected problems occur would help some.

The nsisNoOptimise docs already suggest filing bugs when optimizations introduce bugs. We could also add a note there and to the main nsis function saying something like, "Beware, unsafeInject and unsafeGlobalInject may break the optimizer."

more of the library should be implemented as sugar over injected stuff, without everything gaining an entry in the AST

I like this idea. It would be nice to work toward some intermediary abstraction that's a bit safer, though.

ndmitchell commented 7 years ago

Happy to go with unsafeInject and unsafeInjectGlobal, and we can consider the issue of losing variables later on. I forgot how useful optimisations were to actually debug stuff - a very good point.

Another option would be some kind of unsafeInject :: [Exp String], which can then reference strings - not really sure. My inclination is to merge unsafeInject etc soon, make a release, and potentially refine them later.

jmitchell commented 7 years ago

I've renamed the functions, added some documentation, and resolved the merge conflict with the new Unicode feature.

I opted to go with the straight String -> Action (). Although the fromString implementation for IsString (Exp a) is useful, I feel if we're going to support this sort of raw injection it should go through with minimal transformation. We can always add more convenience utilities above this unsafe mechanism. I'm open to discussing the pros and cons more if you'd like, though. You're more familiar with the project :)

I'd also like to see these changes get released soon. Is there anything I can do to help on that end?

ndmitchell commented 7 years ago

Done, and I released 0.3.1. Agreed with your points on minimal transformation.