✨ NEW: Add line continuation matching and "HERE-documents"

[sappelhoff]

potentially ...

closes #111
closes #119
closes #122
closes #124

if d050f87 is not reverted, see #126 (review) for more context.

---

closes #105

This PR is the execution of what I described in #105 (comment).

It allows copying blocks like:

$ datalad download-url http://www.tldp.org/LDP/Bash-Beginners-Guide/Bash-Beginners-Guide.pdf \
  --dataset . \
  -m "add beginners guide on bash" \
  -O books/bash_guide.pdf

and

$ cat << EOT > notes.txt
   One can create a new dataset with 'datalad create [--description] PATH'.
   The dataset is created empty

   EOT

I am aware that my solution is perhaps not very robust and/or beautiful. I am opening this PR more as a proof of concept and in the hope that somebody will help me to turn this into a proper feature (more robust).

If this is not wanted/deemed helpful for sphinx-copybutton, it would be fine with me to keep it in my fork or transfer it to a fork on the datalad GitHub org. tagging @adswa, who can be a judge whether this proposal would be good for https://github.com/datalad-handbook/book

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

[sappelhoff]

PS: I added two examples prominently to index.html on https://sphinx-copybutton--126.org.readthedocs.build/en/126/ --> that's just to allow quick testing for reviewers! 🙂

[choldgraf]

Thanks for this - a couple quick comments. In general it seems reasonable, though we're hard-coding some assumptions about end of line characters etc. That seems reasonable to me to use \ but I wonder if this could just be a list of "end of line characters" that the user could also over-ride if they wished, and we just loop through the list and use a single variable like foundEndOfLine rather than a different variable each for gotBackslash and gotEOT. What do you think?

[sappelhoff]

. In general it seems reasonable, though we're hard-coding some assumptions about end of line characters etc. That seems reasonable to me to use \ but I wonder if this could just be a list of "end of line characters" that the user could also over-ride if they wished, and we just loop through the list and use a single variable like foundEndOfLine rather than a different variable each for gotBackslash and gotEOT. What do you think?

Yes, thanks! I think it makes sense to offer the user two options:

• endOfLineCharacter, when None, multiline statements are not treated in any way (status quo, default) ... when a character (e.g., \), the logic from this PR applies.

"if we have detected a backslash or an EOT on the previous line, then grab the current line's contents and append to our copied text block"

• and for a second option: hereDocumentDelimiter, when None, "here documents" are not treated in any way (status quo, default) ... when some delimiter is specified (e.g., EOT), then the logic from this PR applies.

"If we detect EOT within a line, we copy all following lines until we find EOT again"

Does that sound good to you? If yes, I could try to give this a go ... it would of course mean exposing two more "options" to the user ... but potentially cover two cases with this that are currently not supported.

[sappelhoff]

Okay, I think I got this in a better state than before now, building on your feedback @choldgraf

Users can now specify two new parameters:

• End of line characters
• "Here document" delimiters

These are basically two separate (but related) features. If these params are not specified, sphinx.copybutton behaves just as before (no breaking changes).

However IF they are specified, they allow more versatile behavior. It's now switched ON in the current PR state, so you can try it out a bit.

https://sphinx-copybutton--126.org.readthedocs.build/en/126/

I also added docs and tests. Looking forward to receiving feedback on this! :-)

[choldgraf]

this seems like a reasonable set of features to add + a good implementation w/ tests and docs. I am inclined to just merge this, since the features are only activated by flags anyway, it shouldn't change the default behavior for anybody unless they explicitly turn it on. Is that right @sappelhoff ? Anything else you wanted to tackle in here?

[amotl]

Dear Stefan,

thank you for your work on this. Chris asked me at #127 (comment) to also have a look at this patch. While I have not reviewed the discussions already happening on this PR yet, I am submitting some comments from my pen, only relating to cosmetic aspects. Please take them merely as suggestions.

Other than squashing all commits into a single one after the review will have converged, I don't see any issues.

With kind regards,
Andreas.

[sappelhoff]

Thanks for the suggestions @amotl I am going to merge them.

[sappelhoff]

Alright, I:

• simplified my code a bit (functionality is identical)
• improved the docs a little
• adjusted the code to follow the new suggestion to use "line continuation" rather than "end of line" as a concept

Finally, in d050f87 I ran npm audit fix, because the package had several vulnerabilities reported (see also #111 #119 #122 #124 )

If you don't want that, please feel free to revert that commit :-) (and then please also edit my OP to remove the "closes" automation for the vulnerability PRs, or tell me to do it)

Other than that, this is good to go from my side! Thanks!

[choldgraf]

nice! @sappelhoff can you confirm that the behavior on the JS side hasn't changed once you updated all of those dependencies?

[sappelhoff]

nice! @sappelhoff can you confirm that the behavior on the JS side hasn't changed once you updated all of those dependencies?

@choldgraf to the best of my knowledge yes, npm audit fix only executes backward compatible fixes so that (apart from security fixes) nothing else is changed. And I literally ran npm audit fix.

Also I clicked basically everything in the local and remote (RTD PR artifact) build (also this "deeper page", whatever it is :-) ), and everything works. So I think it's safe.

[choldgraf]

Nice! The "deeper page" is just there to make sure that the copybutton still works in nested pages.

OK I think this is good to go

[welcome]

Congrats on your first merged pull request in this project! 🎉


Thank you for contributing, we are very proud of you! ❤️

[sappelhoff]

Thanks! Very happy to see this feature in 🚀