Remove blank lines instead of emptying it's contents #667

Bennet-Sunder · 2024-05-28T06:30:12Z

Fixes #664

ketkarameya · 2024-05-28T13:49:54Z

src/models/matches.rs

@@ -51,6 +51,10 @@ pub struct Match {
  #[get_mut]
  #[serde(skip)]
  pub(crate) associated_comments: Vec<Range>,
+  #[get]
+  #[get_mut]
+  #[serde(skip)]


this is not just empty lines right? All leading white spaes?

It's only empty lines, we match leading white spaces that starts with a new line character.
https://github.com/uber/piranha/pull/667/files#diff-b157ce35dcfb24fa5ff9bfb7323302e655455ad2a78bb484fdd2e855d7e740f4R143

Bennet-Sunder · 2024-07-21T15:33:59Z

@dvmarcilio Could you pls have a look at this PR when you have bandwidth.

stefanheule · 2024-07-22T12:21:02Z

@Bennet-Sunder Thanks a lot for the contribution. We are still in the process of testing this internally to make sure it doesn't break any existing use-cases, but will try to merge as soon as possible.

danieltrt · 2024-07-29T18:43:20Z

src/models/unit_tests/source_code_unit_test.rs

+      end_point: Point { row: 1, column: 8 }
+    }
+  );
+}


Hi @Bennet-Sunder ,

Thank you for your contribution! I tested this internally and noticed that the change might break some rewrites. The issue is that in some grammars (particularly Swift), whitespace and newlines are part of the node. If you delete them, you might accidentally delete parts of a sibling node unrelated to the node you're deleting.

Please accept this test suggestion to see what I mean. The test should fail with your change (make sure to add Swift at the top). My proposed solution is to delete only whitespace or newlines until you find the end_byte of the previous sibling node. Let me know if you need help with this.

Suggested change

}

}

#[test]

fn test_switch_entry_blank_lines() {

let inline_cleanup_rule = piranha_rule! {

name= "inline_cleanup_rule",

query= "

(

(switch_entry

(switch_pattern) @p1

(switch_pattern) @p2

(switch_pattern) @p3

(switch_pattern) @p4

(switch_pattern) @p5

(switch_pattern) @p6

) @custom_entry

(#eq? @p1 \".case_zeta_first\")

)

",

replace_node = "custom_entry",

replace = ""

};

let inline_rule = InstantiatedRule::new(&inline_cleanup_rule, &HashMap::new());

let source_code = r#"

public var namespace: ParameterNamespace {

switch self {

case .case_random_word_alpha,

.case_random_word_beta,

.case_random_word_gamma,

.case_random_word_delta:

return .namespace_group_alpha

case .case_zeta_first,

.case_zeta_second,

.case_zeta_third,

.case_zeta_fourth,

.case_zeta_fifth,

.case_zeta_sixth:

return .namespace_group_beta

case .case_random_word_omega:

return .namespace_group_gamma

}

}

"#

.trim();

let piranha_arguments = PiranhaArgumentsBuilder::default()

.paths_to_codebase(vec![UNUSED_CODE_PATH.to_string()])

.language(PiranhaLanguage::from(SWIFT))

.build();

let mut rule_store = RuleStore::new(&piranha_arguments);

let mut parser = piranha_arguments.language().parser();

let mut source_code_unit = SourceCodeUnit::new(

&mut parser,

source_code.to_string(),

&HashMap::new(),

PathBuf::new().as_path(),

&piranha_arguments,

);

source_code_unit.apply_rule(inline_rule, &mut rule_store, &mut parser, &None);

let transformed_code = source_code_unit.code();

let expected_code = r#"

public var namespace: ParameterNamespace {

switch self {

case .case_random_word_alpha,

.case_random_word_beta,

.case_random_word_gamma,

.case_random_word_delta:

return .namespace_group_alpha

case .case_random_word_omega:

return .namespace_group_gamma

}

}

"#

.trim();

assert_eq!(transformed_code, expected_code);

}

danieltrt · 2024-07-29T18:43:40Z

src/models/unit_tests/source_code_unit_test.rs

@@ -16,7 +16,7 @@ use tree_sitter::Parser;
 use crate::{
  filter,
  models::{
-    default_configs::{JAVA, UNUSED_CODE_PATH},
+    default_configs::{JAVA, RUBY, UNUSED_CODE_PATH},


Suggested change

default_configs::{JAVA, RUBY, UNUSED_CODE_PATH},

default_configs::{JAVA, RUBY, SWIFT, UNUSED_CODE_PATH},

Bennet-Sunder · 2024-08-23T03:07:47Z

@stefanheule @danieltrt Thanks for your review of the PR. I can see that a new PR is raised with the fixes. I'll close this PR in favour of the newer one.

Fixes the PR #667 for feature request #664 --------- Co-authored-by: Bennet Sunder <bennet.sunder@freshworks.com>

ketkarameya reviewed May 28, 2024

View reviewed changes

Bennet-Sunder force-pushed the remove_blank_lines branch 2 times, most recently from 0af76d8 to a6c5f6b Compare June 2, 2024 16:37

Bennet-Sunder requested a review from ketkarameya June 2, 2024 16:38

remove blank lines

053f74b

Bennet-Sunder force-pushed the remove_blank_lines branch from a6c5f6b to 053f74b Compare July 21, 2024 15:32

danieltrt suggested changes Jul 29, 2024

View reviewed changes

danieltrt mentioned this pull request Aug 22, 2024

Fix for : Remove blank lines instead of emptying it's contents #687

Merged

Bennet-Sunder closed this Aug 23, 2024

stefanheule pushed a commit that referenced this pull request Sep 2, 2024

Fix for : Remove blank lines instead of emptying it's contents (#687)

7323a98

Fixes the PR #667 for feature request #664 --------- Co-authored-by: Bennet Sunder <bennet.sunder@freshworks.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove blank lines instead of emptying it's contents #667

Remove blank lines instead of emptying it's contents #667

Bennet-Sunder commented May 28, 2024

ketkarameya May 28, 2024

Bennet-Sunder May 28, 2024

Bennet-Sunder commented Jul 21, 2024

stefanheule commented Jul 22, 2024

danieltrt Jul 29, 2024

danieltrt Jul 29, 2024

Bennet-Sunder commented Aug 23, 2024

-}
+}
+#[test]
+fn test_switch_entry_blank_lines() {
+  let inline_cleanup_rule = piranha_rule! {
+    name= "inline_cleanup_rule",
+    query= "
+    (
+        (switch_entry
+            (switch_pattern) @p1
+            (switch_pattern) @p2
+            (switch_pattern) @p3
+            (switch_pattern) @p4
+            (switch_pattern) @p5
+            (switch_pattern) @p6
+        ) @custom_entry
+        (#eq? @p1 \".case_zeta_first\")
+    )
+    ",
+    replace_node = "custom_entry",
+    replace = ""
+  };
+  let inline_rule = InstantiatedRule::new(&inline_cleanup_rule, &HashMap::new());
+  let source_code = r#"
+    public var namespace: ParameterNamespace {
+        switch self {
+        case .case_random_word_alpha,
+             .case_random_word_beta,
+             .case_random_word_gamma,
+             .case_random_word_delta:
+            return .namespace_group_alpha
+        case .case_zeta_first,
+             .case_zeta_second,
+             .case_zeta_third,
+             .case_zeta_fourth,
+             .case_zeta_fifth,
+             .case_zeta_sixth:
+            return .namespace_group_beta
+        case .case_random_word_omega:
+            return .namespace_group_gamma
+        }
+    }
+  "#
+  .trim();
+  let piranha_arguments = PiranhaArgumentsBuilder::default()
+    .paths_to_codebase(vec![UNUSED_CODE_PATH.to_string()])
+    .language(PiranhaLanguage::from(SWIFT))
+    .build();
+  let mut rule_store = RuleStore::new(&piranha_arguments);
+  let mut parser = piranha_arguments.language().parser();
+  let mut source_code_unit = SourceCodeUnit::new(
+    &mut parser,
+    source_code.to_string(),
+    &HashMap::new(),
+    PathBuf::new().as_path(),
+    &piranha_arguments,
+  );
+  source_code_unit.apply_rule(inline_rule, &mut rule_store, &mut parser, &None);
+  let transformed_code = source_code_unit.code();
+  let expected_code = r#"
+    public var namespace: ParameterNamespace {
+        switch self {
+        case .case_random_word_alpha,
+             .case_random_word_beta,
+             .case_random_word_gamma,
+             .case_random_word_delta:
+            return .namespace_group_alpha
+        case .case_random_word_omega:
+            return .namespace_group_gamma
+        }
+    }
+  "#
+  .trim();
+  assert_eq!(transformed_code, expected_code);
+}

	default_configs::{JAVA, RUBY, UNUSED_CODE_PATH},
	default_configs::{JAVA, RUBY, SWIFT, UNUSED_CODE_PATH},

Remove blank lines instead of emptying it's contents #667

Remove blank lines instead of emptying it's contents #667

Conversation

Bennet-Sunder commented May 28, 2024

ketkarameya May 28, 2024

Choose a reason for hiding this comment

Bennet-Sunder May 28, 2024

Choose a reason for hiding this comment

Bennet-Sunder commented Jul 21, 2024

stefanheule commented Jul 22, 2024

danieltrt Jul 29, 2024

Choose a reason for hiding this comment

danieltrt Jul 29, 2024

Choose a reason for hiding this comment

Bennet-Sunder commented Aug 23, 2024