couple of fixes for docstring indentation (PY-2667)

python/src/com/jetbrains/python/PythonDocumentationProvider.java

@@ -2,12 +2,15 @@ package com.jetbrains.python;
 
 import com.intellij.codeInsight.documentation.DocumentationManager;
 import com.intellij.lang.documentation.QuickDocumentationProvider;
-import com.intellij.openapi.diff.impl.patch.PatchLine;
+import com.intellij.openapi.project.Project;
 import com.intellij.openapi.util.Pair;
+import com.intellij.openapi.util.text.LineTokenizer;
+import com.intellij.openapi.util.text.StringUtil;
 import com.intellij.openapi.vfs.VfsUtil;
 import com.intellij.openapi.vfs.VirtualFile;
 import com.intellij.psi.PsiElement;
 import com.intellij.psi.PsiManager;
+import com.intellij.psi.codeStyle.CodeStyleSettingsManager;
 import com.intellij.psi.util.PsiTreeUtil;
 import com.intellij.xml.util.XmlStringUtil;
 import com.jetbrains.python.console.PydevConsoleRunner;
@@ -163,40 +166,51 @@ public class PythonDocumentationProvider extends QuickDocumentationProvider {
     return cat;
   }
 
-  private static @NotNull ChainIterable<String> combUpDocString(@NotNull String docstring) {
+  private static @NotNull ChainIterable<String> combUpDocString(Project project, @NotNull String docstring) {
     ChainIterable<String> cat = new ChainIterable<String>();
     // detect common indentation
-    String[] fragments = Pattern.compile("\n").split(docstring);
+    String[] lines = LineTokenizer.tokenize(docstring, false);
     Pattern spaces_pat = Pattern.compile("^\\s+");
     boolean is_first = true;
-    final int IMPOSSIBLY_BIG = 999999;
-    int cut_width = IMPOSSIBLY_BIG;
-    for (String frag : fragments) {
+    int cut_width = Integer.MAX_VALUE;
+    int firstIndentedLine = 0;
+    for (String frag : lines) {
       if (frag.length() == 0) continue;
       int pad_width = 0;
       final Matcher matcher = spaces_pat.matcher(frag);
       if (matcher.find()) {
         pad_width = matcher.end();
-        if (is_first) {
-          is_first = false;
-          if (pad_width == 0) continue; // first line may have zero padding // first line may have zero padding
+      }
+      if (is_first) {
+        is_first = false;
+        if (pad_width == 0) {    // first line may have zero padding
+          firstIndentedLine = 1;
+          continue;
         }
       }
       if (pad_width < cut_width) cut_width = pad_width;
     }
     // remove common indentation
-    if (cut_width > 0 && cut_width < IMPOSSIBLY_BIG) {
-      for (int i=0; i < fragments.length; i+= 1) {
-        if (fragments[i].length() > 0) fragments[i] = fragments[i].substring(cut_width);
+    if (cut_width > 0 && cut_width < Integer.MAX_VALUE) {
+      for (int i = firstIndentedLine; i < lines.length; i+= 1) {
+        if (lines[i].length() > 0) lines[i] = lines[i].substring(cut_width);
       }
     }
     // reconstruct back, dropping first empty fragment as needed
     is_first = true;
-    for (String frag : fragments) {
-      if (is_first && spaces_pat.matcher(frag).matches()) continue; // ignore all initial whitespace
+    int tabSize = CodeStyleSettingsManager.getSettings(project).getTabSize(PythonFileType.INSTANCE);
+    for (String line : lines) {
+      if (is_first && spaces_pat.matcher(line).matches()) continue; // ignore all initial whitespace
       if (is_first) is_first = false;
       else cat.add(BR);
-      cat.add(combUp(frag));
+      int leadingTabs = 0;
+      while (leadingTabs < line.length() && line.charAt(leadingTabs) == '\t') {
+        leadingTabs++;
+      }
+      if (leadingTabs > 0) {
+        line = StringUtil.repeatSymbol(' ', tabSize * leadingTabs) + line.substring(leadingTabs);
+      }
+      cat.add(combUp(line));
     }
     return cat;
   }
@@ -321,7 +335,7 @@ public class PythonDocumentationProvider extends QuickDocumentationProvider {
         }
       }
       if (docString != null) {
-        doc_cat.add(BR).add(combUpDocString(docString));
+        doc_cat.add(BR).add(combUpDocString(element.getProject(), docString));
       }
     }
     else if (is_property) {
@@ -440,7 +454,7 @@ public class PythonDocumentationProvider extends QuickDocumentationProvider {
             }
             epilog_cat
               .add(BR).add(BR)
-              .addWith(TagCode, combUpDocString(inherited_doc))
+              .addWith(TagCode, combUpDocString(fun.getProject(), inherited_doc))
             ;
             not_found = false;
             break;
@@ -462,7 +476,7 @@ public class PythonDocumentationProvider extends QuickDocumentationProvider {
                 PyStringLiteralExpression predefined_doc_expr = obj_underscored.getDocStringExpression();
                 String predefined_doc = predefined_doc_expr != null? predefined_doc_expr.getStringValue() : null;
                 if (predefined_doc != null && predefined_doc.length() > 1) { // only a real-looking doc string counts
-                  doc_cat.add(combUpDocString(predefined_doc));
+                  doc_cat.add(combUpDocString(fun.getProject(), predefined_doc));
                   epilog_cat.add(BR).add(BR).add(PyBundle.message("QDOC.copied.from.builtin"));
                 }
               }

python/testData/quickdoc/Indented.html

@@ -0,0 +1 @@
+<html><body><code>def <b>foo</b>()<br>Inferred&nbsp;return&nbsp;type:&nbsp;None<br>Doc&nbsp;of&nbsp;foo.<br>It&nbsp;has&nbsp;two&nbsp;lines.</code></body></html>

python/testData/quickdoc/Indented.py

@@ -0,0 +1,7 @@
+# directly in function
+def foo():
+  """<the_doc>Doc of foo.
+  It has two lines."""
+  pass
+
+<the_ref>foo

python/testSrc/com/jetbrains/python/PyQuickDocTest.java

@@ -5,11 +5,13 @@ import com.intellij.openapi.vfs.LocalFileSystem;
 import com.intellij.openapi.vfs.VfsUtil;
 import com.intellij.openapi.vfs.VirtualFile;
 import com.intellij.psi.PsiElement;
+import com.intellij.testFramework.TestDataFile;
 import com.jetbrains.python.fixtures.LightMarkedTestCase;
 import com.jetbrains.python.psi.*;
 import com.jetbrains.python.psi.impl.PythonLanguageLevelPusher;
 
 import java.io.File;
+import java.io.IOException;
 import java.util.Map;
 
 /**
@@ -27,23 +29,33 @@ public class PyQuickDocTest extends LightMarkedTestCase {
     myProvider = new PythonDocumentationProvider();
   }
 
-  private void checkByHTML(String text) throws Exception {
+  private void checkByHTML(String text) {
     assertNotNull(text);
-    String filePath = "/quickdoc/" + getTestName(false) + ".html";
+    checkByHTML(text, "/quickdoc/" + getTestName(false) + ".html");
+  }
+
+  private void checkByHTML(String text, @TestDataFile String filePath) {
     final String fullPath = getTestDataPath() + filePath;
     final VirtualFile vFile = LocalFileSystem.getInstance().findFileByPath(fullPath.replace(File.separatorChar, '/'));
     assertNotNull("file " + fullPath + " not found", vFile);
 
-    String fileText = StringUtil.convertLineSeparators(VfsUtil.loadText(vFile), "\n");
+    String loadedText;
+    try {
+      loadedText = VfsUtil.loadText(vFile);
+    }
+    catch (IOException e) {
+      throw new RuntimeException(e);
+    }
+    String fileText = StringUtil.convertLineSeparators(loadedText, "\n");
     assertEquals(fileText.trim(), text.trim());
   }
 
   @Override
-  protected Map<String, PsiElement> loadTest() throws Exception {
+  protected Map<String, PsiElement> loadTest() {
     return configureByFile("/quickdoc/" + getTestName(false) + ".py");
   }
 
-  private void checkRefDocPair() throws Exception {
+  private void checkRefDocPair() {
     Map<String, PsiElement> marks = loadTest();
     assertEquals(2, marks.size());
     final PsiElement original_elt = marks.get("<the_doc>");
@@ -59,7 +71,7 @@ public class PyQuickDocTest extends LightMarkedTestCase {
     checkByHTML(myProvider.generateDoc(doc_owner, original_elt));
   }
 
-  private void checkHTMLOnly() throws Exception {
+  private void checkHTMLOnly() {
     Map<String, PsiElement> marks = loadTest();
     final PsiElement original_elt = marks.get("<the_ref>");
     PsiElement ref_elt = original_elt.getParent(); // ident -> expr
@@ -67,7 +79,7 @@ public class PyQuickDocTest extends LightMarkedTestCase {
     checkByHTML(myProvider.generateDoc(doc_owner, original_elt));
   }
 
-  private void checkHover() throws Exception {
+  private void checkHover() {
     Map<String, PsiElement> marks = loadTest();
     final PsiElement original_elt = marks.get("<the_ref>");
     PsiElement ref_elt = original_elt.getParent(); // ident -> expr
@@ -75,39 +87,43 @@ public class PyQuickDocTest extends LightMarkedTestCase {
     checkByHTML(myProvider.getQuickNavigateInfo(doc_owner, original_elt));
   }
 
-  public void testDirectFunc() throws Exception {
+  public void testDirectFunc() {
     checkRefDocPair();
   }
 
-  public void testDirectClass() throws Exception {
+  public void testIndented() {
     checkRefDocPair();
   }
 
-  public void testClassConstructor() throws Exception {
+  public void testDirectClass() {
     checkRefDocPair();
   }
 
-  public void testClassUndocumentedConstructor() throws Exception {
+  public void testClassConstructor() {
+    checkRefDocPair();
+  }
+
+  public void testClassUndocumentedConstructor() {
     checkHTMLOnly();
   }
 
-  public void testClassUndocumentedEmptyConstructor() throws Exception {
+  public void testClassUndocumentedEmptyConstructor() {
     checkHTMLOnly();
   }
 
-  public void testCallFunc() throws Exception {
+  public void testCallFunc() {
     checkRefDocPair();
   }
 
-  public void testModule() throws Exception {
+  public void testModule() {
     checkRefDocPair();
   }
 
-  public void testMethod() throws Exception {
+  public void testMethod() {
     checkRefDocPair();
   }
 
-  public void testInheritedMethod() throws Exception {
+  public void testInheritedMethod() {
     Map<String, PsiElement> marks = loadTest();
     assertEquals(2, marks.size());
     PsiElement doc_elt = marks.get("<the_doc>").getParent(); // ident -> expr
@@ -122,11 +138,11 @@ public class PyQuickDocTest extends LightMarkedTestCase {
     checkByHTML(myProvider.generateDoc(doc_owner, null));
   }
 
-  public void testPropNewGetter() throws Exception {
+  public void testPropNewGetter() {
     checkHTMLOnly();
   }
 
-  public void testPropNewSetter() throws Exception {
+  public void testPropNewSetter() {
     Map<String, PsiElement> marks = loadTest();
     PsiElement ref_elt = marks.get("<the_ref>");
     PythonLanguageLevelPusher.setForcedLanguageLevel(myFixture.getProject(), LanguageLevel.PYTHON26);
@@ -139,7 +155,7 @@ public class PyQuickDocTest extends LightMarkedTestCase {
     }
   }
 
-  public void testPropNewDeleter() throws Exception {
+  public void testPropNewDeleter() {
     Map<String, PsiElement> marks = loadTest();
     PsiElement ref_elt = marks.get("<the_ref>");
     PythonLanguageLevelPusher.setForcedLanguageLevel(myFixture.getProject(), LanguageLevel.PYTHON26);
@@ -152,32 +168,32 @@ public class PyQuickDocTest extends LightMarkedTestCase {
     }
   }
 
-  public void testPropOldGetter() throws Exception {
+  public void testPropOldGetter() {
     checkHTMLOnly();
   }
 
 
-  public void testPropOldSetter() throws Exception {
+  public void testPropOldSetter() {
     Map<String, PsiElement> marks = loadTest();
     PsiElement ref_elt = marks.get("<the_ref>");
     final PyDocStringOwner doc_owner = (PyDocStringOwner)((PyTargetExpression)(ref_elt.getParent())).getReference().resolve();
     checkByHTML(myProvider.generateDoc(doc_owner, ref_elt));
   }
 
-  public void testPropOldDeleter() throws Exception {
+  public void testPropOldDeleter() {
     checkHTMLOnly();
   }
 
-  public void testHoverOverClass() throws Exception {
+  public void testHoverOverClass() {
     checkHover();
   }
 
 
-  public void testHoverOverFunction() throws Exception {
+  public void testHoverOverFunction() {
     checkHover();
   }
 
-  public void testHoverOverMethod() throws Exception {
+  public void testHoverOverMethod() {
     checkHover();
   }
 }