Implement sass / scss engines

[emilyriederer]

This PR closes #1665 and adds Sass / SCSS language engines.

When available and not specifically declined by the user (via an r.sass = FALSE chunk option), conversion is handled by LibSass via the sass R package. Otherwise, conversion is done by the standalone DartSass executable found either via PATH or provided via the engine.path option.

Following conversion, output is handled the same as in the css language engine.

[emilyriederer]

@cderv

Let me know if I should go ahead and change the logic to use the sass package as suggested by @yutannihilation's comment . If so, is there a specific way that you would like this block commented to ensure it gets updated after the sass package is release?

I noticed lines 210-211 contain a # TODO: comment for a similar issue related to reticulate

[cderv]

I left some comment to discuss.

Also, I have a behaviour rather strange that we should cover.
I installed sass executable on my Windows. Something was not correct and it does not work. So I should have error printing but nothing. Instead, sass.exe does not error because the cmd works, but the conversion does not happen and it sends as output the error message. So following your code, in out I got

'dart.exe' n'est pas reconnu en tant que commande interne
ou externe, un programme ex‚cutable ou un fichier de commandes.

and as it is not an error, I get in my rendered html

<style type="text/css">
'dart.exe' n'est pas reconnu en tant que commande interne
ou externe, un programme ex‚cutable ou un fichier de commandes.
</style>

The html does open but I don't think this something desirable.
We should instead find a way to catch the error message from saas executable. I think with system2(), we should test that returned value has not a status error code. (see ?system2) with xfun::attr(out, "status").

I don't know if you have the same not on windows.

As a general comment also, this need some documentation, maybe first as a bullet point in NEWS.

[emilyriederer]

Thanks @cderv for the review! And apologies -- there are definitely a few things in there that you shouldn't have had to catch. A few follow-up questions:

• I think I have the changes ready now. Do you have a preference how I add them (e.g. more commits versus force pushing an amended version of the initial commit)? Looking back at past PRs in this repo, I couldn't get a strong sense
• I also have begun to think that it would be better to have the default CSS output be compressed (no whitespace) for the (admittedly minimal) efficiency it provides. It's trivial to add a sass.compressed chunk option so that users could set sass.compressed = FALSE to get expanded (and more human-readable) output.
  • Do you agree with the decision for compression to be the default?
  • Is it worth adding an additional chunk option or is the potential utility too low to justify introducing a new option?
• Thank you for the call-out on system2 status codes versus R errors. In retrospect, this need should have been obvious to me based on the eng_interpreted code. I couldn't replicate the specific "dart.exe" error you had on either my Windows 8 or 10, but I was able to induce other status code-related errors to test my fix so I believe it should work now.

Per the last comment, I'll also include an updated NEWS.md with the revised PR.

[cderv]

Just a quick reply as it is late but that will keep you going

I think I have the changes ready now. Do you have a preference how I add them (e.g. more commits versus force pushing an amended version of the initial commit)? Looking back at past PRs in this repo, I couldn't get a strong sense

Do not force push as it is not necessary. When we can avoid I think it is better. We will deal with that later. You can add more commits to this branch. We should be able to "squash and merge" the PR, in order to reduce the amount of commit. Or I'll deal myself with git stuff at the end - I am knew as a reviewer for this 📦, so we both learn in the process 😄

I also have begun to think that it would be better to have the default CSS output be compressed (no whitespace) for the (admittedly minimal) efficiency it provides. It's trivial to add a sass.compressed chunk option so that users could set sass.compressed = FALSE to get expanded (and more human-readable) output.

If you want to try add this, feel free. I don't think css does "compress" the output, nor it is minimal in pandoc template. Do we have a big gain compare to the cost of maintaining ?
Does sass package and sass exe can both handle this ? Is this a command line argument you add ?
I don't know if it should be the default, but it could be interesting to pass arguments to sass engine through the engine.opts options. Look at this option in engine.R.
If a user wants to change the default, it just needs knitr option settings.

Thank you for the call-out on system2 status codes versus R errors. In retrospect, this need should have been obvious to me based on the eng_interpreted code. I couldn't replicate the specific "dart.exe" error you had on either my Windows 8 or 10, but I was able to induce other status code-related errors to test my fix so I believe it should work now.

Don't worry about dart.exe. I know how to reproduce so I could test when you are ready to share code.

[emilyriederer]

Thanks for taking the time to reply!

If you want to try add this, feel free. I don't think css does "compress" the output, nor it is minimal in pandoc template. Do we have a big gain compare to the cost of maintaining ?

The css engine definitely doesn't naturally. I thought perhaps this could be one other use case for the scss engine - to provide normal css and have the whitespace stripped. Admittedly, for most single-file knitr uses, I can't really imagine the performance boost would be noticeable unless someone is doing something particularly long and complicated (at which point it might be a better idea to convert their Sass externally anyway?). However, there's also limited downside. I don't think maintenance is a real problem since its outsourced to either LibSass or Dart Sass, but there is also definitely a readability tradeoff.

Does sass package and sass exe can both handle this ? Is this a command line argument you add ?

Yes! Both versions can handle it.

I don't know if it should be the default, but it could be interesting to pass arguments to sass engine through the engine.opts options. Look at this option in engine.R.
If a user wants to change the default, it just needs knitr option settings.

Thanks - that's a good point. It really does make more sense for this to be an engine option than a chunk option. I'll update this in the code before pushing my new commits.

[cderv]

I let you push your changes and we'll see what default to choose. 🤔

[emilyriederer]

Hi @cderv ,

I just pushed a number of changes. Let me know what you think!

I'm still not sure if I love the API for output styling, so definitely open to more feedback here. In the end I considered four options. Happy to switch if you think one of the others is better.

sass sass.package = FALSE, sass.compressed = FALSE

Most readable / typeable but felt "aggressive" somehow to add a new chunk option for something so specialized.

sass sass.package = FALSE, engine.opts = 'expanded' (<- how it works now)

Feels weird to not have the option labeled "style" in anyway, but that is also unneeded since there is only one option. This felt most in keeping with the main usage of engine.opts to pass additional command line arguments (although I pass 'expanded' versus '--style=expanded' so the syntax seems reasonable for either the R package or sass implementation.)

sass sass.package = FALSE, engine.opts = list(style = 'expanded')

Most explicit but also unnecessarily verbose.

sass engine.opts = list(package = FALSE, style = 'expanded')

Most compact but no longer follows same convention as python.reticulate (engine implementation as chunk option)

---

On an unrelated note, there are a lot of degrees of freedom now with two engines, possible options, and different types of warnings / errors. I haven't included tests in the PR as I notice knitr doesn't have them for most engines (and, of course, the set-up on Travis is non-trivial). That said, if it would ease your job for me to share tests (either through the PR or as an attachment?), please just let me know how you'd like me to send.

[cderv]

I have still made a few comment, specifically on the error and warning catching. I think we should reproduce more closely eng_interpreted. I let you read those and see. If you want me to add some commit on this one, as I have the dart.exe error to test, please tell me.

About the style API, I agree with you it could be improved. one thing is we want the option to work for both package and executable sass. So we can't have a TRUE vs FALSE, if the package have more option for styling.

The cleaner API would be the first : sass.package and sass.style chunk option. The more close to knitr current behavior would be the last, as engine.opts. Some knit engine works that way.
interpreted engine use engine.opts as a way to provide additional command line options. Some others use it to pass engine specific options. (like tikz)
I guess both are okay, it just need some documentation or examples. Mixing both option as of now may be a bit too complicated for little gain.

On an unrelated note, there are a lot of degrees of freedom now with two engines, possible options, and different types of warnings / errors. I haven't included tests in the PR as I notice knitr doesn't have them for most engines (and, of course, the set-up on Travis is non-trivial). That said, if it would ease your job for me to share tests (either through the PR or as an attachment?), please just let me know how you'd like me to send.

knitr 📦 has a very specific workflow for tests. the integration test (among them engines) are done using knitr_example repo. All the file in this repo is reknit each time a travis build is done. Pretty clever, but also a not-so-simple workflow 😄
To test the sxss engine, it would require a new file in the repo (could be a way to add documentation too) and some adjustment in travis to get the executable. I think we could do that afterward - I can work on the integration and you could work on the example file (that you may already have). Will see as soon as we are ok on this PR.

[emilyriederer]

Hi @cderv,

I've just pushed a number of updates:

• Moved all options into engine.opts to be more consistent with knitr
• Updated engine selection so that if engine.path is provided, the executable is used regardless of whether or not package = FALSE (since clearly this seems to be the user intent)
• Changed handling of system2() call to explicitly check for status, as in eng_interpreted()

While these aren't "official" tests or in the style of knitr, I'm also including my local testthat script (as txt versus R, per GitHub limitations) in case it is at all helpful. I know there's a lot to look over with all of the sources of variations (package vs executable) x (style) x (error options). While it doesn't include your specific errors, I am able to trigger some status code issues to test handling there.

At minimum, hopefully this restores a bit of confidence after I know I've made some pretty stupid mistakes; I'm very grateful for your help and patience!

test_eng_sxss.txt

[cderv]

I see you used warning2 below 👍 (I did not know it)
Should we use stop2 also here ? The wrapper with call. = FALSE.
As you prefer.

[emilyriederer]

Ah! Very nice. I hadn't noticed that wrapper as much in the other engines, but I like it. I will switch over. Thanks for the recommendation!

[cderv]

Thank you very much !!

This was not a small PR and it is really good! Thanks a lot ! You did an awesome job all along.

This is working with my error now, and your test file is awesome. It could be added in in the test folder, but it is using testit and not testthat. But the file was useful to check this PR. Thank you !

I just pushed a small commit to be closer to knit internal code style.

Also, I left 2 small comments. It is more like question I let you deal with.
After you give me an answer, I will merge it.

Thanks again !

[cderv]

what is out_tagged ? it does not seem to be used.
Did I miss something ? 🤔

[emilyriederer]

Ah, thanks for catching. You didn't -- I did. Previously I had more validation steps after this, but it appears I failed to remove this assignment when I took those out.

[emilyriederer]

Hi @cderv ,

Thanks for the last polishing thoughts! I switched to stop2 throughout and removed the extraneous variable assignment. Also appreciate the styling changes. I tried to stay consistent to knitr but definitely missed some things along the way. (I guess my mental energy there was too focused on repeatedly changing all my <-s to =s ? 🙂 )

Please let me know if there's anything else I should do on my end, like eventually adding examples to the knitr_examples repo, etc. I'll also keep an eye out for sass's CRAN release so we can remove the calls to get().

Thank you again so, so much for your help and great feedback at every step of the way!

[cderv]

Awesome thank you ! I'll need to check if I missed anything in the process. It is my first review for knitr 📦 and first time merging - I'll do it very soon.

I will also add you as contributor to the 📦 in DESCRIPTION.
Thanks a lot ! This will be a super new feature! 🎉

[cderv]

@yihui, review is done. and this is working well. Just checking how you get things done usually.

From what I observed until now

• I added @emilyriederer as contributor because she wasn't
• I increased the version number in DESCRIPTION as it is new feature

Is this ready to squash and merge ?

As it is my first review for knitr, I prefer to go through this time to learn the workflow for this 📦 and be at ease next time ☺️

Thanks.

[yihui]

Everything looks great. Please go ahead and make your first merge in this repo! 🎉 Thank you both!

@emilyriederer Next time please feel free to use <- if you struggle with =. I'll change it by myself after the PR is merged.

[yihui]

Good eye. Thanks!

[emilyriederer]

Thanks, @yihui! And don't worry -- I was just joking, not complaining, about = and <-. I really didn't mind! After all, a blogger I really admire once wrote a great post about adhering to package style in PRs? 😄 It 100% makes sense, and that's actually the easiest one of all since its so readily apparent from the rest of the code that that is the preference. I just wish I'd done a better job noticing some of the other things that @cderv caught!

Thanks again to you both!