Correction of the work with comments and general improvements (#163)

* Changed the if to switch construction in the Rules method.

* The duplicate code has been removed.

* Simplified the atruleNames function

* Minor changes.

* Added a fix for not deleting comments

* Changing the tests

Added a test to check the work with comments

The existing tests have been changed because they contained incorrect data.

* Added code for processing comments in the rules.

* Updated tests.

* Added processing for new lines.

* Updated tests, added the use of new lines

* The readability of the test has been improved.

* Added a test to save the father's comment

* Edit to correct the lint error.

Author: Ulyanov-programmer

index.js

@@ -53,12 +53,12 @@ function interpolateAmpInSelector(nodes, parent) {
  */
 function mergeSelectors(parent, child) {
   let merged = []
-  parent.selectors.forEach(sel => {
+  for (let sel of parent.selectors) {
     let parentNode = parse(sel, parent)
 
-    child.selectors.forEach(selector => {
+    for (let selector of child.selectors) {
       if (!selector) {
-        return
+        continue
       }
       let node = parse(selector, child)
       let replaced = interpolateAmpInSelector(node, parentNode)
@@ -67,22 +67,43 @@ function mergeSelectors(parent, child) {
         node.prepend(parentNode.clone({}))
       }
       merged.push(node.toString())
-    })
-  })
+    }
+  }
   return merged
 }
 
 /**
- * Move a child and its preceeding comment(s) to after "after"
+ * Move a child and its preceding comment(s) to after "after"
+ * ! It is necessary to clarify the comment
  */
 function breakOut(child, after) {
-  let prev = child.prev()
-  after.after(child)
-  while (prev && prev.type === 'comment') {
-    let nextPrev = prev.prev()
-    after.after(prev)
-    prev = nextPrev
+  let changeParent = true
+
+  for (let node of after.nodes) {
+    if (!node.nodes) continue
+
+    let prevNode = node.prev()
+    if (prevNode?.type !== 'comment') continue
+
+    let parentRule = after.toString()
+
+    /* Checking that the comment "describes" the rule following. Like this:
+      /* comment about the rule below /*
+      .rule {}
+    */
+    let regexp = new RegExp(`${prevNode.toString()} *\n *${node.toString()}`)
+
+    if (parentRule.match(regexp)) {
+      changeParent = false
+      after.after(node).after(prevNode)
+    }
+  }
+
+  // It is necessary if the above child has never been moved
+  if (changeParent) {
+    after.after(child)
   }
+
   return child
 }
 
@@ -104,14 +125,12 @@ function createFnAtruleChilds(bubble) {
         children.push(child)
       }
     })
-    if (bubbling) {
-      if (children.length) {
-        let clone = rule.clone({ nodes: [] })
-        for (let child of children) {
-          clone.append(child)
-        }
-        atrule.prepend(clone)
+    if (bubbling && children.length) {
+      let clone = rule.clone({ nodes: [] })
+      for (let child of children) {
+        clone.append(child)
       }
+      atrule.prepend(clone)
     }
   }
 }
@@ -125,16 +144,23 @@ function pickDeclarations(selector, declarations, after) {
   after.after(parent)
   return parent
 }
+function pickAndClearDeclarations(ruleSelector, declarations, after, clear = true) {
+  if (!declarations.length) return [after, declarations]
 
-function atruleNames(defaults, custom) {
-  let list = {}
-  for (let name of defaults) {
-    list[name] = true
+  after = pickDeclarations(ruleSelector, declarations, after)
+
+  if (clear) {
+    declarations = []
   }
-  if (custom) {
-    for (let name of custom) {
-      list[name.replace(/^@/, '')] = true
-    }
+
+  return [after, declarations]
+}
+
+function atruleNames(defaults, custom = '') {
+  let names = defaults.concat(custom)
+  let list = {}
+  for (let name of names) {
+    list[name.replace(/^@/, '')] = true
   }
   return list
 }
@@ -297,10 +323,10 @@ module.exports = (opts = {}) => {
     postcssPlugin: 'postcss-nested',
 
     RootExit(root) {
-      if (root[hasRootRule]) {
-        root.walkAtRules(rootRuleName, unwrapRootRule)
-        root[hasRootRule] = false
-      }
+      if (!root[hasRootRule]) return
+
+      root.walkAtRules(rootRuleName, unwrapRootRule)
+      root[hasRootRule] = false
     },
 
     Rule(rule) {
@@ -310,46 +336,48 @@ module.exports = (opts = {}) => {
       let declarations = []
 
       rule.each(child => {
-        if (child.type === 'rule') {
-          if (declarations.length) {
-            after = pickDeclarations(rule.selector, declarations, after)
-            declarations = []
-          }
-
-          copyDeclarations = true
-          unwrapped = true
-          child.selectors = mergeSelectors(rule, child)
-          after = breakOut(child, after)
-        } else if (child.type === 'atrule') {
-          if (declarations.length) {
-            after = pickDeclarations(rule.selector, declarations, after)
-            declarations = []
-          }
-          if (child.name === rootRuleName) {
-            unwrapped = true
-            atruleChilds(rule, child, true, child[rootRuleMergeSel])
-            after = breakOut(child, after)
-          } else if (bubble[child.name]) {
-            copyDeclarations = true
-            unwrapped = true
-            atruleChilds(rule, child, true)
-            after = breakOut(child, after)
-          } else if (unwrap[child.name]) {
+        switch (child.type) {
+          case 'rule':
+            [after, declarations] = pickAndClearDeclarations(rule.selector, declarations, after)
+
             copyDeclarations = true
             unwrapped = true
-            atruleChilds(rule, child, false)
+            child.selectors = mergeSelectors(rule, child)
             after = breakOut(child, after)
-          } else if (copyDeclarations) {
-            declarations.push(child)
-          }
-        } else if (child.type === 'decl' && copyDeclarations) {
-          declarations.push(child)
+
+            break
+          case 'atrule':
+            [after, declarations] = pickAndClearDeclarations(rule.selector, declarations, after)
+
+            if (child.name === rootRuleName) {
+              unwrapped = true
+              atruleChilds(rule, child, true, child[rootRuleMergeSel])
+              after = breakOut(child, after)
+            } else if (bubble[child.name]) {
+              copyDeclarations = true
+              unwrapped = true
+              atruleChilds(rule, child, true)
+              after = breakOut(child, after)
+            } else if (unwrap[child.name]) {
+              copyDeclarations = true
+              unwrapped = true
+              atruleChilds(rule, child, false)
+              after = breakOut(child, after)
+            } else if (copyDeclarations) {
+              declarations.push(child)
+            }
+
+            break
+          case 'decl':
+            if (copyDeclarations) {
+              declarations.push(child)
+            }
+
+            break
         }
       })
 
-      if (declarations.length) {
-        after = pickDeclarations(rule.selector, declarations, after)
-      }
+      pickAndClearDeclarations(rule.selector, declarations, after, false)
 
       if (unwrapped && preserveEmpty !== true) {
         rule.raws.semicolon = true

index.test.js

@@ -604,16 +604,47 @@ test('clears empty selector after comma', () => {
   run('a, b { .one, .two, {} }', 'a .one, a .two, b .one, b .two {}')
 })
 
-test('moves comment with rule', () => {
-  run('a { /*B*/ /*B2*/ b {} }', '/*B*/ /*B2*/ a b {}')
+test("Save the parent's comment", () => {
+  run('a { /*i*/ b {} }', 'a { /*i*/ } a b {}')
 })
 
-test('moves comment with at-rule', () => {
-  run('a { /*B*/ @media { one: 1 } }', '/*B*/ @media {a { one: 1 } }')
+test("Save the parent's comment with newline", () => {
+  run(
+    `a { 
+    /*i*/
+
+     b {} }`,
+    `a { /*i*/ } a b {}`
+  )
+})
+
+test('Save the comments for the parent and child', () => {
+  run(
+    `a { 
+    /*i*/ 
+    /*o*/
+    b {} }`,
+
+    `a { /*i*/ } /*o*/ a b {}`
+  )
+})
+
+test('Save the comments for the parent and child with at-rule', () => {
+  run(
+    `a { /*i*/ 
+    /*o*/
+    @media { one: 1 } }`,
+
+    `a { /*i*/ } /*o*/ @media {a { one: 1 } }`
+  )
 })
 
 test('moves comment with declaration', () => {
-  run('a { @media { /*B*/ one: 1 } }', '@media {a { /*B*/ one: 1 } }')
+  run('a { @media { /*B*/ one: 1 } }', '@media { a { /*B*/ one: 1 } }')
+})
+
+test('moves comment with declaration without properties', () => {
+  run('a { @media { /*B*/ } }', '@media { a { /*B*/ } }')
 })
 
 test('saves order of rules', () => {