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

Keep all backslashes in domain directives. #6473

Merged
merged 5 commits into from Mar 21, 2020

Conversation

jakobandersen
Copy link
Contributor

@jakobandersen jakobandersen commented Jun 11, 2019

Subject: modify the central line handler for domain directives to not touch double backslashes.

Feature or Bugfix

  • Bugfix

Purpose

It looks like a long time ago an extra regex replacement was inserted in function that handles escaped line breaks in domain directives. It replaces two consecutive backslashes with a single backlash. This means that users/tools must escape all backslashes in declarations once more. Breathe doesn't do this, does autodoc?
This PR changes the default behaviour so not backslash stripping is being done by default.
If a domain is specifically designed to rely on the stripping, it can set strip_backslashes = True in the subclasses of ObjectDescription.
Users can selectively reenable the stripping in specific domains with a new configuration value signature_backslash_strip_domain_override (a list of domain names). The default is None, while the empty list [] reverts all domains to the old behaviour.

Relates

@codecov
Copy link

codecov bot commented Jun 11, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@a3a775b). Click here to learn what that means.
The diff coverage is 35.71%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6473   +/-   ##
=========================================
  Coverage          ?   84.37%           
=========================================
  Files             ?      267           
  Lines             ?    40420           
  Branches          ?     5935           
=========================================
  Hits              ?    34106           
  Misses            ?     5003           
  Partials          ?     1311
Impacted Files Coverage Δ
sphinx/directives/__init__.py 84.5% <35.71%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3a775b...581f425. Read the comment docs.

@tk0miya
Copy link
Member

tk0miya commented Jun 13, 2019

This was added at 10 year ago (bd32a22). I don't know how useful this stripping. If this was introduced only for helping editors' highlighting as commented, I think this is worse approach because this brings inconsistency to our notation. But this code has stripped backslashes in signatures during 10 years. That makes many signatures escaped. As a result, this change is a breaking change (or destructive change) even if this is right approach.

Therefore, we need to switch this behavior carefully. So it would be better to merge this into major release. In addition, it would be better to provide migration path like some warnings, configurations and so on.

@shimizukawa do you have any opinion?

@t-b
Copy link

t-b commented Jun 25, 2019

@tk0miya @jakobandersen
(I'm the reporter of the bug)

Sphinx does not get tripped in 1.8.3 so this looks like a regression to me. Or did the C++ domain get better and this error is only now exposed?

@jakobandersen jakobandersen changed the base branch from 2.0 to master October 6, 2019 15:26
@jakobandersen
Copy link
Contributor Author

jakobandersen commented Oct 6, 2019

@t-b, I'm quite sure that it is not due to changes in backslash handling. Rather I think it may be because the C++ domain has started doing more detailed parsing of string literals.

@jakobandersen
Copy link
Contributor Author

@tk0miya, good points. I have change the PR to include various ways to get the old behaviour (see the updated PR description and the code changes). How does it look to you?

@t-b
Copy link

t-b commented Feb 2, 2020

@jakobandersen Works as advertised. Thanks for the PR!

@t-b
Copy link

t-b commented Feb 18, 2020

@jakobandersen @tk0miya Can this be part of the 3.0 release?

@jakobandersen
Copy link
Contributor Author

jakobandersen commented Mar 8, 2020

Ping @tk0miya, if you find time before 3.0 freeze.

@tk0miya tk0miya added this to the 3.0.0 milestone Mar 16, 2020
@tk0miya
Copy link
Member

tk0miya commented Mar 16, 2020

Oh, sorry for not responding longer.

TBH, I hesitate to add such a complex implementation to the core. Because I suppose nobody (or only very few people) uses this behavior (because nobody knows it!). So I'd like to add a small migration code during 1 or 2 major versions. And dropping codes by historical reason finally. For such purpose, this is too much to support old behavior.

If my understanding correct, this feature only affects users. And no object-descriptions depends on it because it is a preprocessor before they work. So I suppose no options like strip_backslashes are not needed. How about add simply signature_backslash_strip = True | False to keep compatibility? It only enables/disables the stripping backslashes (defaults to False). It's much simpler than now.

@jakobandersen
Copy link
Contributor Author

@tk0miya, if not even autodoc relies on this I completely agree. I'll implement your suggestion soon and update the PR.

@tk0miya
Copy link
Member

tk0miya commented Mar 18, 2020

Basically autodoc does not depend on the behavior. But I found an exceptional case:

Input:

def foo(name='\\'):
    pass

Output:

.. py:function:: foo(name='\\\\')
   :module: example

It uses the stripping to suppress escaped backslash. I'll take a look from now on.

@tk0miya
Copy link
Member

tk0miya commented Mar 18, 2020

I posted #7334 as an example for this. But it seems better to drop stripping backslashes also for autodoc. What do you think?

@tk0miya
Copy link
Member

tk0miya commented Mar 18, 2020

Sorry, please forget my comment above.

autodoc expects the stripping in ObjectDescription and do escaping in 2 methods. So this should be disabled by signature_backslash_strip setting.

# escape backslashes for reST
args = args.replace('\\', '\\\\')
return args

# escape backslashes for reST
args = args.replace('\\', '\\\\')
return args

@jakobandersen jakobandersen changed the base branch from master to 3.x March 20, 2020 12:34
@jakobandersen
Copy link
Contributor Author

The code has now been simplified and the two cases in autodoc @tk0miya found are now conditioned on the stripping being active.

There is a test case in test_autodoc.py that now fails. @tk0miya, do you know how easily to split this particular one into two cases: one with stripping and one without? Otherwise I can simplify it to only test the new behaviour.

@tk0miya
Copy link
Member

tk0miya commented Mar 20, 2020

+1 for testing with new behavior only.

@tk0miya
Copy link
Member

tk0miya commented Mar 21, 2020

LGTM, merging now!

@tk0miya tk0miya merged commit 5054fd9 into sphinx-doc:3.x Mar 21, 2020
@jakobandersen jakobandersen deleted the 6462_backslashes branch March 24, 2020 12:44
openandclose added a commit to openandclose/tosixinch that referenced this pull request Aug 23, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants