Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'add_row_if' and 'add_rows_if' methods #106

Merged
merged 6 commits into from
May 16, 2023
Merged

Add 'add_row_if' and 'add_rows_if' methods #106

merged 6 commits into from
May 16, 2023

Conversation

Techassi
Copy link
Contributor

This PR adds the ability to only add (a) row(s) when the specified predicate evaluates to true. This enables a smoother "flow" when building tables. For example:

let mut table = Table::new();
table
    .set_header(&vec!["Header1", "Header2", "Header3"])
    .add_row(&vec![
        "This is a text",
        "This is another text",
        "This is the third text",
    ]);
if predicate {
    table.add_row(vec!["", "", ""]);
}
table
    .add_row(vec!["", "", ""])
    .add_row(vec!["", "", ""]);

With the changes introduced by this PR, we could write the above code like:

let mut table = Table::new();
table
    .set_header(&vec!["Header1", "Header2", "Header3"])
    .add_row(&vec![
        "This is a text",
        "This is another text",
        "This is the third text",
    ]);
    .add_row_if(|| predicate, vec!["", "", ""])
    .add_row(vec!["", "", ""])
    .add_row(vec!["", "", ""]);

The same feature is implemented for add_rows_if. I also added some tests, which should cover the newly added feature.

@Techassi
Copy link
Contributor Author

Is there anything blocking this PR? Is there anything I can do to push this forward @Nukesor?

@Nukesor
Copy link
Owner

Nukesor commented Apr 28, 2023

Hey :)

Sorry for being so slow to respond. I'm currently super busy and I rarely find time to maintain my open source projects.

This will hopefully change in a week or so. We got some public holiday coming up this weekend, so I might get a bit of time to review a few things :)

@Nukesor
Copy link
Owner

Nukesor commented Apr 29, 2023

I think this could be neat, but I don't like that the fact that the predicate doesn't take any parameters at all.
Right now, you could just accept a boolean instead of a closure and pre-compute the boolean beforehand.

What do you think about the following to make this a bit more useful:
The predicate could get the row/rows as an argument. The user could then decide whether to ignore or use that info and it would allow users to easily hide rows based on their content and respective filter functions.

@Techassi
Copy link
Contributor Author

Techassi commented May 3, 2023

Yeah I was thinking the same thing, the predicate doesn't do much at the moment. I was also thinking about passing in some kind of "context". I like the idea of passing in row information! I will look into it and will update this PR accordingly.

The closure function now provides the current row index and a reference
to all rows as parameters accessible inside the closure body.
@Techassi
Copy link
Contributor Author

Techassi commented May 3, 2023

I updated the closure function signature from

Fn() -> bool

to

Fn(usize, &Vec<Row>) -> bool

Users now have access the the current row index and all previously added rows. Let me know, if these parameters make sense.

@Techassi
Copy link
Contributor Author

Can we move forward with these changes @Nukesor?

src/table.rs Outdated Show resolved Hide resolved
The predicate function now takes the current index (unchanged) and the
to be added row as the context.

This commit also adds two new tests, which make use of the newly passed
row.
@Techassi Techassi requested a review from Nukesor May 12, 2023 10:17
@Techassi
Copy link
Contributor Author

Let me know if there is anything left to add to this PR @Nukesor before merging!

Copy link
Owner

@Nukesor Nukesor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice :)

I think I'm happy with the current state. Thanks for your contribution and sorry for taking so long 😅

@Nukesor Nukesor merged commit 997414f into Nukesor:main May 16, 2023
@Techassi
Copy link
Contributor Author

No worries! Love to hear that!

@Techassi Techassi deleted the feat/add-if branch May 18, 2023 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants