Add define block skip

[Sigmanificient]

Hi, I tried to implement the define block skip myself, as it is a feature that I would like to have.

• Resolves Subscripts detected as runnable targets #236

Tested on glibc's Makefile, and seems skip define properly.

Note

With some background in C, I'm still unfamiliar with Rust, so I focused on a simple implementation that works. It might not be fancy, and not be the best way to pull this off, so I rely on your expertise for some feedback.

[kyu08]

@Sigmanificient
Thank you for exploring and dissecting the code, and for the effort to submit a PR despite not being familiar with Rust! ☺️
I will aim to respond within the next few days. Just hold on a bit! 🙇‍♂️

[kyu08]

Sorry for the late reply. I spent time researching whether if-endif, ifdef-endif and ifeq-endif should be treated as a start of block to ignore, but I found out they should not be treated as such.

I would like you to address the following points.
If you feel it's difficult, feel free to ask me. I don't mind adding some commits to this PR myself.
Of course, if you want to accomplish it on your own, I'll support you.

1. About override directives

According to the specification of GNU make, define accepts override directive before it.
So, please treat override define as start of block to ignore too.

2. About nested define directives

According to the specification of GNU make, define directives can be nested.

You may nest define directives: make will keep track of nested directives and report an error if they are not all properly closed with endef. Note that lines beginning with the recipe prefix character are considered part of a recipe, so any define or endef strings appearing on such a line will not be considered make directives.

How about counting the "current depth of define directives" and ignoring lines where the depth is greater than 0, as shown below?

diff --git a/src/model/target.rs b/src/model/target.rs
index af82b55..2c81761 100644
--- a/src/model/target.rs
+++ b/src/model/target.rs
@@ -4,13 +4,30 @@ use regex::Regex;
 
 use super::file_util;
 
+const START_OF_DEFINE_BLOCK: &str = "define";
+const END_OF_DEFINE_BLOCK: &str = "endef";
+
 #[derive(Debug, Clone, PartialEq)]
 pub struct Targets(pub Vec<String>);
 
 impl Targets {
     pub fn new(content: String) -> Targets {
         let mut result: Vec<String> = Vec::new();
+        let mut ignored_block_count = 0;
+
         for line in content.lines() {
+            if line.trim() == START_OF_DEFINE_BLOCK {
+                ignored_block_count += 1;
+                continue;
+            }
+            if line.trim() == END_OF_DEFINE_BLOCK {
+                ignored_block_count -= 1;
+            }
+
+            if 0 < ignored_block_count {
+                continue;
+            }
+
             if let Some(t) = line_to_target(line.to_string()) {
                 result.push(t);
             }

3. About Unit tests

Please add some unit tests to content_to_targets_test. (My naming is not good.😅 It should be names like makefile_new_test or something.
Test cases to add may be:

• Makefile content including a define directive
• Makefile content including a override define directive
• Makefile content including a nested define directive

If you have any questions, feel free to ask me!

[Sigmanificient]

Including both may add quite some complexity to the initial parsing function.
So, in an effort to keep readability, maybe I should try to extract a get_line_type function

enum LineType { Normal, DefineStart, DefineEnd }

fn get_line_type(line: &str) -> LineType {
    let words: Vec<&str> = line.split_whitespace().collect();

    if (words.len() >= 2
        && words[0] == OVERRIDE
        && words[1] == DEFINE_BLOCK_START) {
        return LineType::DefineStart;
    }

    match line.trim() {
        DEFINE_BLOCK_START => LineType::DefineStart,
        DEFINE_BLOCK_END => LineType::DefineEnd,
        _ => LineType::Normal,
    }
}

Something like this?

[Sigmanificient]

About the unit testing, shouldn't they be placed in their own function define_block_skip_test?

[kyu08]

About unit test

About the unit testing, shouldn't they be placed in their own function define_block_skip_test?

I agree. The unit tests should be placed in get_line_type_test.

So it seems that get_line_type_test should have test cases like this:

1. Return Normal when the line is empty
2. Return DefineStart when the line starts with override define
3. Return DefineStart when the line equals define
4. Return DefineEnd when the line equals endef
5. Return Normal when the line is something else

About code format

• Please run cargo fmt. (I'm sorry that the check in the CI were not properly set up.)

[Sigmanificient]

Hello again, I updated the source according to the change you requested!

I also made sure to format it properly, adding the required rustfmt to the dev shell in the way.
Then I finally added the unit tests, along some whitespace one, just in case. Had to put #[derive(Debug, PartialEq)] macros above the Enum to make it work.

Hopefully it will be good this time!

[kyu08]

@Sigmanificient
Great work! 👍
Again, thank you for sending a PR despite not being familiar with Rust!
Your contributions have made fzf-make more robust.
The idea to use enums was also brilliant. 👏

I would be delighted if you would contribute again in the future!
Thank you so much!

[kyu08]

@Sigmanificient
This fix released v0.27.0.
https://github.com/kyu08/fzf-make/releases/tag/v0.27.0

[Sigmanificient]

Splendid! Just pr-ed the version bump to nixpkgs 👀

NixOS/nixpkgs#301279