sphinx-copybutton icon indicating copy to clipboard operation
sphinx-copybutton copied to clipboard

Conflict between regex exclusion/prompt selection and copybutton_exclude

Open rkdarst opened this issue 3 years ago • 12 comments

From https://github.com/executablebooks/sphinx-copybutton/pull/178#issuecomment-1318492169 .

Describe the bug

There are now two ways to avoid copying prompts and output:

  1. copybutton_exclude excludes certain CSS classes. Sphinx uses this by default to prevent selection of prompts.
  2. copybutton_prompt_text, copybutton_only_copy_prompt_lines, copybutton_remove_prompts, copybutton_copy_empty_lines.

(1) is in theory automatic: the Pygments lexer should highlights prompts with the class .gp, and outputs with the class .go. Right now, the default exclusion is .linenos, .gp. You have to remember to use the right lexer, for example "Python console" instead of "Python" or "console" instead of "bash".

(2) is manually configured to certain programming languages, per Sphinx project.

The conflict is that (1) removes the prompt text, so that certain features of (2) don't see the prompt regex, so that copybutton_only_copy_prompt_lines no longer works by default. Workarounds:

  • copybutton_exclude = '.linenos' or
  • (I haven't check if this works) add .go to copybutton_exclude and set copybutton_copy_empty_lines = False

I can think of two strategies to do:

a) undo the chaneg in defaults, switch copybutton_exclude to .linenos by default (this previous default). In order to exclude prompt and output, everyone needs to configure it by either strategy (1) or (2) above. Document these two options and the choices. Disadvantages: doesn't work out of the box for anyone, not even "a bit better" than default. Copybutton has worse default behavior than Sphinx.

b) Go further towards copybutton_exclude. Set default to .linenos, .gp, .go to exclude the console output, too (or document how to do this). Set copybutton_copy_empty_lines = False by default. Encourage people to use the right lexers and fix broken lexers (sphinx has good support for this). Documentation is updated for how to set `copybutton_exclude '.linenos' to turn this off and only use with regexps.

  • Disadvantages: diverges from the default Sphinx behavior (it does .linesnos, .gp). There are valid reasons to copy the output, and not being able to do this is confusing (but more so than not being able to copy the prompt? It assumes no one wants to copy outputs, which probably isn't true, but one could always change the lexer to plain text if one wants it to not be treated as console).
  • Advantages: by default, if you specify the right language, it Just Works.
  • TODO: But we need to check the "copy bduplicated lines" thing.

Incidentally, I thought that pygments included an IPython lexer, but couldn't find it. In fact, it seems that it does - but is distributed with IPython itself, and seems to use Generic.Prompt and Generic.Output.

I will update this description as I find more facts.

Reproduce the bug

Use the things above, I will try to create a test case later.

List your environment

latest release

rkdarst avatar Nov 17 '22 14:11 rkdarst

https://github.com/executablebooks/sphinx-copybutton/pull/186 is a demonstration of solution (b) where it goes all-in on using Pygments to detect everything. It mostly works, but:

  • here documents don't work
  • Based on #178, there was almost a solution, but blank lines were copied. copybutton_copy_empty_lines = False in theory would solev this, but
    • it only worked when the prompt regexes were defined, because
    • if it worked all the time, it could exclude other important blank lines
    • so the change in that PR excludes other blank lines which would be relevant.

rkdarst avatar Nov 17 '22 16:11 rkdarst

@hassec, as a reference, in your case, what lexer do you use for these code blocks? If you use ipythonconsole, and change to ipython, does it work? (no, you shouldn't do this, but an interesting test)

rkdarst avatar Nov 17 '22 16:11 rkdarst

After thinking about this, testing, and writing all of the above, here are my thoughts on what I would do (sort of a null option):

  • Leave things as they are now. The reason for this is it is as close to upstream sphinx as possible (least surprise), and works for any language reasonably well with no configuration needed.
  • Document that for copybutton_prompt_text, it is best to set copybutton_exclude='.linenos'.
  • Document setting copybutton_exclude = '.linenos, .gp, .go' as a useful setting, though imperfect.
  • Arrange these different sections about excluding text closer together.

Hopefully, in the future, someone could make more systematic improvements. It's better to rely on the real lexers rather than re-invent things with regexes, but regexes are very useful as a fallback for advanced cases. Unfortunately, I don't expect to have time to do much more than improve the docs. Perhaps the regex-based system can somehow be more closely integrated with the Pygments system so that it can mostly rely on that, but fix it up some.

rkdarst avatar Nov 17 '22 16:11 rkdarst

https://github.com/executablebooks/sphinx-copybutton/pull/187 is an immediate docs fix while discussing the best option.

rkdarst avatar Nov 17 '22 17:11 rkdarst

Sorry for the slow reply @rkdarst, busy day. But 👍 for the prompt action.

My example case was just a .. code:: python or doctest block in sphinx, with highlighting via pygments.

FWIW, I think the default should change back to copybutton_exclude = '.lineos'. Everyone who's currently using this plugin with copybutton_prompt_text will have broken copy buttons after updating and that's annoying because it won't even be that obvious and will likely not get caught for a while.

hassec avatar Nov 17 '22 18:11 hassec

Is your sample project available online - from what I can tell, the python lexer shouldn't output .gp.

But I agree with your sentiment about not breaking working things, and it's a bit of a shame we have to choose between them. I still somewhat lean towards "those who configured it once can configure again, those who haven't configured anything don't have to configure anything." I wonder what the general balances of users is, though - how many configure? how many don't? how many use it through something else (jupyter-book?) that assumes some configuration?

But @choldgraf can decide and I can make the PR, if it's easy.

rkdarst avatar Nov 17 '22 19:11 rkdarst

No, unfortunately it isn't. But I can easily reproduce it:

Empty folder with empty conf.py and a index.rst with:

>>> print(1+2)
3

and run sphinx-build -Eanb html . build/ which will produce a page that has the .gp for the prompt and .go for the output.

"those who configured it once can configure again, those who haven't configured anything don't have to configure anything."

I disagree, because those who configured it are getting a silent breaking change they might not even notice and those who didn't configure are getting different behavior now. Both scenarios are cases that users don't expect in a patch version update IMHO.

hassec avatar Nov 17 '22 20:11 hassec

Yeah, you are right, there should'nt be a change without some expectation - at least not without planning. #188 changes back, after that I will further update the #187 to reflect the new state of things.

I found why it's .gp: I could'nt see it in pygments, but Sphinx tries to be clever and tests node.rawsource.startswith('>>>') and makes those pythonconsole. Perhaps to make docstring-based stuff work better.

rkdarst avatar Nov 17 '22 20:11 rkdarst

Reference example.

btw Can a final character be added to copied content?

flywire avatar Dec 19 '22 20:12 flywire

I think #178 has a silent breaking change as what hassec mentioned, the default config or sample config from the document doesn't work anymore. The output keeps getting copied.

As a workaround, adding .go to copybutton_exclude will break multi-line snippet.

Sample bugs display: https://sphinx-copybutton-exclude-issue.readthedocs.io/en/latest/ (you can see how v0.5.1, v0.5.1 with .go added and v0.5.0)

thyhum avatar Jan 13 '23 12:01 thyhum

Hi -

SQLAlchemy here. I just noticed that copybutton 0.5.1 on our site introduces a regression in this area vs. 0.5.0; our regex is no longer matching.

I'm going to read over these changes but it seems there was definitely a breaking change in 0.5.1 in this area.

zzzeek avatar Feb 09 '23 16:02 zzzeek

OK, good feature but IMO adding .gp to the default classes was a mistake. it directly contradicts the use of copybutton_prompt_text. I would assume lots of sites that use copybutton are currently broken since nobody knew about this breaking change.

zzzeek avatar Feb 09 '23 16:02 zzzeek