PY-16758 Preserve order of sections specified in Google code style guidelines

python/src/com/jetbrains/python/documentation/GoogleCodeStyleDocString.java

@@ -31,7 +31,7 @@ import java.util.regex.Pattern;
  * @see <a href="http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Comments#Comments">Google Python Style: Docstrings</a>
  */
 public class GoogleCodeStyleDocString extends SectionBasedDocString {
-  public static final Pattern SECTION_HEADER_RE = Pattern.compile("\\s*(\\w+):\\s*", Pattern.MULTILINE);
+  public static final Pattern SECTION_HEADER_RE = Pattern.compile("\\s*([\\w\\s]+):\\s*", Pattern.MULTILINE);
   private static final Pattern FIELD_NAME_AND_TYPE_RE = Pattern.compile("\\s*(.+?)\\s*\\(\\s*(.*?)\\s*\\)\\s*");
 
   public GoogleCodeStyleDocString(@NotNull Substring text) {

python/src/com/jetbrains/python/documentation/SectionBasedDocString.java

@@ -89,7 +89,7 @@ public abstract class SectionBasedDocString extends DocStringLineParser implemen
   private static final ImmutableSet<String> SECTIONS_WITH_NAME = ImmutableSet.of(METHODS_SECTION);
 
   @Nullable
-  protected static String normalizeSectionTitle(@NotNull @NonNls String title) {
+  public static String getNormalizedSectionTitle(@NotNull @NonNls String title) {
     return SECTION_ALIASES.get(title.toLowerCase());
   }
 
@@ -148,7 +148,7 @@ public abstract class SectionBasedDocString extends DocStringLineParser implemen
     if (pair.getFirst() == null) {
       return Pair.create(null, sectionStartLine);
     }
-    final String normalized = normalizeSectionTitle(pair.getFirst().toString());
+    final String normalized = getNormalizedSectionTitle(pair.getFirst().toString());
     if (normalized == null) {
       return Pair.create(null, sectionStartLine);
     }
@@ -497,15 +497,20 @@ public abstract class SectionBasedDocString extends DocStringLineParser implemen
   }
 
   @NotNull
-  private List<Section> getSectionsWithNormalizedTitle(@NotNull final String title) {
+  public List<Section> getSectionsWithNormalizedTitle(@NotNull final String title) {
     return ContainerUtil.mapNotNull(mySections, new Function<Section, Section>() {
       @Override
       public Section fun(Section section) {
-        return section.getNormalizedTitle().equals(title) ? section : null;
+        return section.getNormalizedTitle().equals(getNormalizedSectionTitle(title)) ? section : null;
       }
     });
   }
 
+  @Nullable
+  public Section getFirstSectionWithNormalizedTitle(@NotNull String title) {
+    return ContainerUtil.getFirstItem(getSectionsWithNormalizedTitle(title));
+  }
+
   @Nullable
   @Override
   public String getAttributeDescription() {
@@ -534,7 +539,7 @@ public abstract class SectionBasedDocString extends DocStringLineParser implemen
     @NotNull
     public String getNormalizedTitle() {
       //noinspection ConstantConditions
-      return normalizeSectionTitle(getTitle());
+      return getNormalizedSectionTitle(getTitle());
     }
 
     @NotNull

python/src/com/jetbrains/python/documentation/docstrings/DocStringBuilder.java

@@ -34,14 +34,24 @@ public abstract class DocStringBuilder<This extends DocStringBuilder> {
 
   @NotNull
   public This addLine(@NotNull String line) {
-    myLines.add(line);
+    return addLine(line, myLines.size());
+  }
+
+  @NotNull
+  public This addLine(@NotNull String line, int index) {
+    myLines.add(index, line);
     //noinspection unchecked
     return (This)this;
   }
 
   @NotNull
   public This addEmptyLine() {
-    return addLine("");
+    return addLine("", myLines.size());
+  }
+
+  @NotNull
+  public This addEmptyLine(int index) {
+    return addLine("", index);
   }
 
   @NotNull

python/src/com/jetbrains/python/documentation/docstrings/GoogleCodeStyleDocStringUpdater.java

@@ -28,7 +28,7 @@ public class GoogleCodeStyleDocStringUpdater extends SectionBasedDocStringUpdate
   }
 
   @Override
-  void updateParamDeclarationWithType(@NotNull Substring nameSubstring, @NotNull String type) {
+  protected void updateParamDeclarationWithType(@NotNull Substring nameSubstring, @NotNull String type) {
     insert(nameSubstring.getEndOffset(), " (" + type + ")");
   }
 

python/src/com/jetbrains/python/documentation/docstrings/NumpyDocStringUpdater.java

@@ -30,12 +30,12 @@ public class NumpyDocStringUpdater extends SectionBasedDocStringUpdater {
   }
 
   @Override
-  void updateParamDeclarationWithType(@NotNull Substring nameSubstring, @NotNull String type) {
+  protected void updateParamDeclarationWithType(@NotNull Substring nameSubstring, @NotNull String type) {
     insert(nameSubstring.getEndOffset(), " : " + type);
   }
 
   @Override
-  protected int getSectionLastTitleLine(@NotNull Section section) {
+  protected int getSectionTitleLastLine(@NotNull Section section) {
     return getSectionStartLine(section) + 1;
   }
 

python/src/com/jetbrains/python/documentation/docstrings/SectionBasedDocStringUpdater.java

@@ -15,7 +15,9 @@
  */
 package com.jetbrains.python.documentation.docstrings;
 
+import com.google.common.collect.ImmutableList;
 import com.intellij.openapi.util.Condition;
+import com.intellij.openapi.util.Pair;
 import com.intellij.openapi.util.text.StringUtil;
 import com.intellij.util.containers.ContainerUtil;
 import com.jetbrains.python.documentation.SectionBasedDocString;
@@ -27,12 +29,23 @@ import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 /**
  * @author Mikhail Golubev
  */
 public abstract class SectionBasedDocStringUpdater extends DocStringUpdater<SectionBasedDocString> {
+  private static final ImmutableList<String> CANONICAL_SECTION_ORDER = ImmutableList.of(
+    SectionBasedDocString.PARAMETERS_SECTION,
+    SectionBasedDocString.KEYWORD_ARGUMENTS_SECTION,
+    SectionBasedDocString.OTHER_PARAMETERS_SECTION,
+    SectionBasedDocString.YIELDS_SECTION,
+    SectionBasedDocString.RETURNS_SECTION,
+    SectionBasedDocString.RAISES_SECTION
+  );
+
   private final List<AddParameter> myAddParameterRequests = new ArrayList<AddParameter>();
 
   public SectionBasedDocStringUpdater(@NotNull SectionBasedDocString docString, @NotNull String minContentIndent) {
@@ -66,18 +79,15 @@ public abstract class SectionBasedDocStringUpdater extends DocStringUpdater<Sect
       }
       else {
         final String newLine = createReturnLine(type, getSectionIndent(returnSection), getExpectedFieldIndent());
-        insertAfterLine(getSectionLastTitleLine(returnSection), newLine);
+        insertAfterLine(getSectionTitleLastLine(returnSection), newLine);
       }
     }
     else {
-      final int line = findLastNonEmptyLine();
-      final String newSection = createBuilder()
+      final SectionBasedDocStringBuilder builder = createBuilder()
         .withSectionIndent(getExpectedFieldIndent())
-        .addEmptyLine()
         .startReturnsSection()
-        .addReturnValue(null, type, "")
-        .buildContent(getExpectedSectionIndent(), true);
-      insertAfterLine(line, newSection);
+        .addReturnValue(null, type, "");
+      insertNewSection(builder, SectionBasedDocString.RETURNS_SECTION);
     }
   }
 
@@ -123,11 +133,9 @@ public abstract class SectionBasedDocStringUpdater extends DocStringUpdater<Sect
       final Section firstParamSection = findFirstParametersSection();
       // Insert whole new parameter block
       if (firstParamSection == null) {
-        paramBlockBuilder
-          .addEmptyLine()
-          .startParametersSection();
-        final String blockText = buildBlock(paramBlockBuilder, newParams, getExpectedFieldIndent(), getExpectedSectionIndent());
-        insertAfterLine(findLastNonEmptyLine(), blockText);
+        paramBlockBuilder.startParametersSection();
+        final SectionBasedDocStringBuilder builder = addParametersInBlock(paramBlockBuilder, newParams, getExpectedFieldIndent());
+        insertNewSection(builder, SectionBasedDocString.PARAMETERS_SECTION);
       }
       // Update existing parameter block
       else {
@@ -135,7 +143,7 @@ public abstract class SectionBasedDocStringUpdater extends DocStringUpdater<Sect
         // Section exist, but empty
         if (firstParamField == null) {
           final String blockText = buildBlock(paramBlockBuilder, newParams, getExpectedFieldIndent(), getSectionIndent(firstParamSection));
-          insertAfterLine(getSectionLastTitleLine(firstParamSection), blockText);
+          insertAfterLine(getSectionTitleLastLine(firstParamSection), blockText);
         }
         else {
           // Section contain other parameter declarations
@@ -153,14 +161,70 @@ public abstract class SectionBasedDocStringUpdater extends DocStringUpdater<Sect
                                    @NotNull List<AddParameter> params,
                                    @NotNull String sectionIndent,
                                    @NotNull String indent) {
+    return addParametersInBlock(builder, params, sectionIndent).buildContent(indent, true);
+  }
+
+  private static SectionBasedDocStringBuilder addParametersInBlock(@NotNull SectionBasedDocStringBuilder builder,
+                                                                   @NotNull List<AddParameter> params,
+                                                                   @NotNull String sectionIndent) {
     builder.withSectionIndent(sectionIndent);
     for (AddParameter param : params) {
       builder.addParameter(param.name, param.type, "");
     }
-    return builder.buildContent(indent, true);
+    return builder;
+  }
+
+  private void insertNewSection(@NotNull SectionBasedDocStringBuilder builder, @NotNull String sectionTitle) {
+    final Pair<Integer, Boolean> pos = findPreferredSectionLine(sectionTitle);
+    if (pos.getSecond()) {
+      builder.addEmptyLine(0);
+      insertAfterLine(pos.getFirst(), builder.buildContent(getExpectedSectionIndent(), true));
+    }
+    else {
+      builder.addEmptyLine();
+      insertBeforeLine(pos.getFirst(), builder.buildContent(getExpectedSectionIndent(), true));
+    }
+  }
+
+  /**
+   * @return pair (lineNum, insertAfter), i.e. first item is line number,
+   * second item is true if new section should be inserted after this line and false otherwise
+   */
+  private Pair<Integer, Boolean> findPreferredSectionLine(@NotNull String sectionTitle) {
+    final String normalized = SectionBasedDocString.getNormalizedSectionTitle(sectionTitle);
+    final int index = CANONICAL_SECTION_ORDER.indexOf(normalized);
+    if (index < 0) {
+      return Pair.create(findLastNonEmptyLine(), true);
+    }
+    final Map<String, Section> namedSections = new HashMap<String, Section>();
+    for (Section section : myOriginalDocString.getSections()) {
+      final String normalizedTitle = section.getNormalizedTitle();
+      // leave only first occurrences
+      if (!namedSections.containsKey(normalizedTitle)) {
+        namedSections.put(normalizedTitle, section);
+      }
+    }
+    for (int i = index - 1; i >= 0; i--) {
+      final Section previous = namedSections.get(CANONICAL_SECTION_ORDER.get(i));
+      if (previous != null) {
+        return Pair.create(getSectionEndLine(previous), true);
+      }
+    }
+    for (int i = index + 1; i < CANONICAL_SECTION_ORDER.size(); i++) {
+      final Section next = namedSections.get(CANONICAL_SECTION_ORDER.get(i));
+      if (next != null) {
+        return Pair.create(getSectionStartLine(next), false);
+      }
+    }
+    return Pair.create(findLastNonEmptyLine(), true);
   }
 
 
+  protected abstract void updateParamDeclarationWithType(@NotNull Substring nameSubstring, @NotNull String type);
+
+  protected abstract SectionBasedDocStringBuilder createBuilder();
+
+  @Nullable
   private Substring findParamNameSubstring(@NotNull final String name) {
     return ContainerUtil.find(myOriginalDocString.getParameterSubstrings(), new Condition<Substring>() {
       @Override
@@ -170,14 +234,10 @@ public abstract class SectionBasedDocStringUpdater extends DocStringUpdater<Sect
     });
   }
 
-  abstract void updateParamDeclarationWithType(@NotNull Substring nameSubstring, @NotNull String type);
-
-  protected int getSectionLastTitleLine(@NotNull Section paramSection) {
+  protected int getSectionTitleLastLine(@NotNull Section paramSection) {
     return getSectionStartLine(paramSection);
   }
 
-  protected abstract SectionBasedDocStringBuilder createBuilder();
-
   protected String createReturnLine(@NotNull String type,
                                     @NotNull String docStringIndent,
                                     @NotNull String sectionIndent) {
@@ -242,6 +302,12 @@ public abstract class SectionBasedDocStringUpdater extends DocStringUpdater<Sect
     return section.getTitleAsSubstring().getStartLine();
   }
 
+  protected int getSectionEndLine(@NotNull Section section) {
+    final List<SectionField> fields = section.getFields();
+    //noinspection ConstantConditions
+    return fields.isEmpty() ? getSectionTitleLastLine(section) : getFieldEndLine(ContainerUtil.getLastItem(fields));
+  }
+
   protected int getFieldStartLine(@NotNull SectionField field) {
     return chooseFirstNotNull(field.getNameAsSubstring(),
                               field.getTypeAsSubstring(),
@@ -261,7 +327,7 @@ public abstract class SectionBasedDocStringUpdater extends DocStringUpdater<Sect
         return value;
       }
     }
-    throw new NullPointerException("At least one of values should be not null");
+    throw new NullPointerException("At least one of values must be not null");
   }
 
   private static class AddParameter {

python/testData/docstrings/googleKeywordArgumentsSection.py

@@ -0,0 +1,5 @@
+def f():
+    """
+    
+    Keyword arguments:
+    """

python/testData/intentions/afterGoogleReturnSectionAfterKeywords.py

@@ -0,0 +1,8 @@
+def f(**kwargs):
+    """
+    Keyword arguments:
+        foo: bar
+
+    Returns:
+        object: 
+    """

python/testData/intentions/afterGoogleReturnSectionAfterYields.py

@@ -0,0 +1,13 @@
+def f():
+    """
+    Yields:
+        int: meaning of life, universe and everything
+
+    Returns:
+        object: 
+        
+    Example:
+        print(next(f))
+    """
+    yield 42
+    return

python/testData/intentions/afterGoogleReturnSectionBeforeRaises.py

@@ -0,0 +1,10 @@
+def f():
+    """
+    Returns:
+      object: 
+
+    Raises:
+        RuntimeException
+    """
+    raise RuntimeException
+    

python/testData/intentions/afterParamSectionBeforeKeywords.py

@@ -0,0 +1,12 @@
+def f(x):
+    """
+    
+    Args:
+      x: 
+
+    Keyword arguments:
+        
+    Returns:
+        None
+    """
+    

python/testData/intentions/afterParamTypeInGoogleDocStringOtherSectionExists.py

@@ -2,9 +2,9 @@ def f(x, y):
     """
     Summary.
     
-    Returns:
-      Something
-
     Args:
       x (object): 
+
+    Returns:
+      Something
     """

python/testData/intentions/afterParamTypeInNumpyDocStringOtherSectionExists.py

@@ -2,11 +2,11 @@ def f(x, y):
     """
     Summary.
     
-    Returns
-    -------
-      Something
-
     Parameters
     ----------
       x : object
+
+    Returns
+    -------
+      Something
     """

python/testData/intentions/beforeGoogleReturnSectionAfterKeywords.py

@@ -0,0 +1,5 @@
+def <caret>f(**kwargs):
+    """
+    Keyword arguments:
+        foo: bar
+    """

python/testData/intentions/beforeGoogleReturnSectionAfterYields.py

@@ -0,0 +1,10 @@
+def <caret>f():
+    """
+    Yields:
+        int: meaning of life, universe and everything
+        
+    Example:
+        print(next(f))
+    """
+    yield 42
+    return

python/testData/intentions/beforeGoogleReturnSectionBeforeRaises.py

@@ -0,0 +1,7 @@
+def <caret>f():
+    """
+    Raises:
+        RuntimeException
+    """
+    raise RuntimeException
+    

python/testData/intentions/beforeParamSectionBeforeKeywords.py

@@ -0,0 +1,9 @@
+def <caret>f(x):
+    """
+    
+    Keyword arguments:
+        
+    Returns:
+        None
+    """
+    

python/testSrc/com/jetbrains/python/PySectionBasedDocStringTest.java

@@ -283,6 +283,13 @@ public class PySectionBasedDocStringTest extends PyTestCase {
     assertSize(1, paramSection.getFields());
   }
 
+  public void testGoogleKeywordArgumentsSection() {
+    final GoogleCodeStyleDocString docString = findAndParseGoogleStyleDocString();
+    assertEmpty(docString.getSummary());
+    assertSize(1, docString.getSections());
+    assertEquals("keyword arguments", docString.getSections().get(0).getNormalizedTitle());
+  }
+
   @Override
   protected String getTestDataPath() {
     return super.getTestDataPath() + "/docstrings";

python/testSrc/com/jetbrains/python/intentions/PyIntentionTest.java

@@ -499,6 +499,26 @@ public class  PyIntentionTest extends PyTestCase {
     doDocReturnTypeTest(DocStringFormat.GOOGLE);
   }
 
+  // PY-16758
+  public void testGoogleReturnSectionAfterKeywords() {
+    doDocReturnTypeTest(DocStringFormat.GOOGLE);
+  }
+  
+  // PY-16758
+  public void testGoogleReturnSectionAfterYields() {
+    doDocReturnTypeTest(DocStringFormat.GOOGLE);
+  }
+
+  // PY-16758
+  public void testGoogleReturnSectionBeforeRaises() {
+    doDocReturnTypeTest(DocStringFormat.GOOGLE);
+  }
+
+  // PY-16758
+  public void testParamSectionBeforeKeywords() {
+    doDocAddMissingParamsTest(DocStringFormat.GOOGLE);
+  }
+
   // PY-9795
   public void testGoogleDocStubWithTypes() {
     final PyCodeInsightSettings codeInsightSettings = PyCodeInsightSettings.getInstance();