Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conflict between regex exclusion/prompt selection and copybutton_exclude #185

Open
rkdarst opened this issue Nov 17, 2022 · 12 comments
Open
Labels
bug Something isn't working

Comments

@rkdarst
Copy link
Collaborator

rkdarst commented Nov 17, 2022

From #178 (comment) .

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 rkdarst added the bug Something isn't working label Nov 17, 2022
rkdarst added a commit to rkdarst/sphinx-copybutton that referenced this issue Nov 17, 2022
- Remove the prompt regexes and set `copybutton_exclude='.linenos,
  .gp, .go'`.  We get a similar behaviour as before, but with no
  custom configuration and would (in theory) work with any
  Pygments-supported language.

- This is a demonstration of solution (b) in executablebooks#185.  We don't define
  prompt or output regexes, but let Pygments lex it and give it css
  classes.  Here, we exclude Generic.Prompt and Generic.Output
  classes.

- The formatCopyText function had to be adjusted, since the "remove
  blank lines" only worked if the prompt detection was on.  This
  re-ordering enables it all the time, which isn't great, since it can
  remove other meaningful blank lines.  It needs to be thought out
  better.

- The IPython and bash lexers are changed to their respective console
  lexers.

- Here documents don't seem to work.  Pygments could be improved to
  support this better.

- Further discussion is in executablebooks#185.  This should not be merged.
@rkdarst
Copy link
Collaborator Author

rkdarst commented Nov 17, 2022

#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 ENH: Exclude unselectable text from being copied (update) #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
Copy link
Collaborator Author

rkdarst commented Nov 17, 2022

@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
Copy link
Collaborator Author

rkdarst commented Nov 17, 2022

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 added a commit to rkdarst/sphinx-copybutton that referenced this issue Nov 17, 2022
- Improves executablebooks#185 by documenting things, but there are likely better
  fixes later.
- Does not fix that issue, just makes it more clear in the short term.
- Review: worth a read to make sure it isn't too confusing.
@rkdarst
Copy link
Collaborator Author

rkdarst commented Nov 17, 2022

#187 is an immediate docs fix while discussing the best option.

@hassec
Copy link

hassec commented Nov 17, 2022

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.

@rkdarst
Copy link
Collaborator Author

rkdarst commented Nov 17, 2022

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.

@hassec
Copy link

hassec commented Nov 17, 2022

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.

rkdarst added a commit to rkdarst/sphinx-copybutton that referenced this issue Nov 17, 2022
- Immediate fix to backwards compatibility of executablebooks#185.
- I'm not implying I wouldn't want to change the default later.
@rkdarst
Copy link
Collaborator Author

rkdarst commented Nov 17, 2022

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 added a commit to rkdarst/sphinx-copybutton that referenced this issue Nov 18, 2022
- As in executablebooks#185, document the current sate of copybutton_exclude and saw
  how to turn it on (since the default just got turned off for
  backwards compat).
- Review: worth a read to make sure it isn't too confusing.
@flywire
Copy link
Contributor

flywire commented Dec 19, 2022

Reference example.

btw Can a final character be added to copied content?

@thyhum
Copy link

thyhum commented Jan 13, 2023

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)

@zzzeek
Copy link

zzzeek commented Feb 9, 2023

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
Copy link

zzzeek commented Feb 9, 2023

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.

sqlalchemy-bot pushed a commit to sqlalchemy/sqlalchemy that referenced this issue Feb 9, 2023
sphinx-copybutton introduced a new feature
in 0.5.1 which includes a default configuration
that breaks the regexp prompt matching scheme.

set copybutton_exclude to not include ".gp" as that's the class
where we exactly look for the prompts we are matching.
While we're there, use this new feature to exclude our sql
styles, even though this is not strictly necessary in our case.

pin sphinx-copybutton at 0.5.1 to avoid future problems.

Change-Id: I8eaeab13995c032b9ee3afd1f08dae5929009d45
References: executablebooks/sphinx-copybutton#185
sqlalchemy-bot pushed a commit to sqlalchemy/sqlalchemy that referenced this issue Feb 9, 2023
sphinx-copybutton introduced a new feature
in 0.5.1 which includes a default configuration
that breaks the regexp prompt matching scheme.

set copybutton_exclude to not include ".gp" as that's the class
where we exactly look for the prompts we are matching.
While we're there, use this new feature to exclude our sql
styles, even though this is not strictly necessary in our case.

pin sphinx-copybutton at 0.5.1 to avoid future problems.

Change-Id: I8eaeab13995c032b9ee3afd1f08dae5929009d45
References: executablebooks/sphinx-copybutton#185
(cherry picked from commit 13d3b2c)
sqlalchemy-bot pushed a commit to sqlalchemy/sqlalchemy that referenced this issue Feb 9, 2023
sphinx-copybutton introduced a new feature
in 0.5.1 which includes a default configuration
that breaks the regexp prompt matching scheme.

set copybutton_exclude to not include ".gp" as that's the class
where we exactly look for the prompts we are matching.
While we're there, use this new feature to exclude our sql
styles, even though this is not strictly necessary in our case.

pin sphinx-copybutton at 0.5.1 to avoid future problems.

Change-Id: I8eaeab13995c032b9ee3afd1f08dae5929009d45
References: executablebooks/sphinx-copybutton#185
(cherry picked from commit 13d3b2c)
(cherry picked from commit faa20b1)
sqlalchemy-bot pushed a commit to sqlalchemy/alembic that referenced this issue Feb 9, 2023
sphinx-copybutton introduced a new feature
in 0.5.1 which includes a default configuration
that breaks the regexp prompt matching scheme.

set copybutton_exclude to not include ".gp" as that's the class
where we exactly look for the prompts we are matching.

pin sphinx-copybutton at 0.5.1 to avoid future problems.

Change-Id: Ie03bc27a9190e71b63fc68b484f23e53b8cb72dc
References: executablebooks/sphinx-copybutton#185
choldgraf added a commit that referenced this issue Apr 14, 2023
* docs/use: Improve docs about copybutton_exclude

- As in #185, document the current sate of copybutton_exclude and saw
  how to turn it on (since the default just got turned off for
  backwards compat).
- Review: worth a read to make sure it isn't too confusing.

* Update docs/use.md

---------

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants