Skip to content

Make wordcount filter work with core only#480

Merged
Kijewski merged 3 commits intoaskama-rs:masterfrom
GuillaumeGomez:wordcount-core
Jun 14, 2025
Merged

Make wordcount filter work with core only#480
Kijewski merged 3 commits intoaskama-rs:masterfrom
GuillaumeGomez:wordcount-core

Conversation

@GuillaumeGomez
Copy link
Copy Markdown
Collaborator

Part of #472.

@Kijewski
Copy link
Copy Markdown
Member

The builtin module contains all filters that only need core, but yeah, I think for consistency it would be a good idea to rename it into core.

Can you adapt ↓ to test if the changed code works well with the partial inputs, too?

fn test_indent_chunked() {
#[derive(Clone, Copy)]
struct Chunked<'a>(&'a str);
impl<'a> fmt::Display for Chunked<'a> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
for chunk in self.0.chars() {
write!(f, "{}", chunk)?;
}
Ok(())
}
}

@GuillaumeGomez
Copy link
Copy Markdown
Collaborator Author

Renamed builtin.rs into core.rs and added the partial input test as well.

@GuillaumeGomez GuillaumeGomez requested a review from Kijewski June 13, 2025 09:24
Comment on lines +594 to +614
if s.is_empty() {
return Ok(());
} else if s.trim().is_empty() {
self.0.ends_with_whitespace = true;
return Ok(());
}
self.0.count += s.split_whitespace().count();
if !self.0.ends_with_whitespace && !s.starts_with(char::is_whitespace) {
self.0.count -= 1;
}
self.0.ends_with_whitespace = s.ends_with(char::is_whitespace);
Ok(())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had a bit of a problem understanding what was going on, so I refactored the code a bit:

Suggested change
if s.is_empty() {
return Ok(());
} else if s.trim().is_empty() {
self.0.ends_with_whitespace = true;
return Ok(());
}
self.0.count += s.split_whitespace().count();
if !self.0.ends_with_whitespace && !s.starts_with(char::is_whitespace) {
self.0.count -= 1;
}
self.0.ends_with_whitespace = s.ends_with(char::is_whitespace);
Ok(())
if s.is_empty() {
return Ok(());
}
// first cut of all whitespaces from the front
let trimmed_s = s.trim_start();
let started_with_withspace = trimmed_s.len() != s.len();
let s = trimmed_s;
if s.is_empty() {
// there was nothing but space in the input
self.0.ends_with_whitespace = true;
return Ok(());
}
let space_in_between = started_with_withspace || self.0.ends_with_whitespace;
// then cut of all whitespaces from the back
let trimmed_s = s.trim_end();
self.0.ends_with_whitespace = trimmed_s.len() != s.len();
let s = trimmed_s;
// `s` cannot be empty. Otherwise `s.trim_start()` would have been empty.
// split at whitespaces and decrement word count if there was no space in between the calls
self.0.count += s.split_whitespace().count();
if !space_in_between {
self.0.count -= 1;
}
Ok(())

The logic is the same, but in many more lines. You decide if it's an improvement. :)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not a big fan but definitely gonna add more code comments.

Kijewski
Kijewski previously approved these changes Jun 13, 2025
Copy link
Copy Markdown
Member

@Kijewski Kijewski left a comment

Choose a reason for hiding this comment

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

Thank you!

@GuillaumeGomez
Copy link
Copy Markdown
Collaborator Author

I added code comments. Please tell me if:

  1. They're understandable.
  2. If they make the logic clear enough.

Copy link
Copy Markdown
Member

@Kijewski Kijewski left a comment

Choose a reason for hiding this comment

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

Looks good. The comments are understandable to me. :) Thank you!

@Kijewski Kijewski merged commit 477bfe4 into askama-rs:master Jun 14, 2025
42 checks passed
@Kijewski Kijewski mentioned this pull request Jun 14, 2025
@GuillaumeGomez GuillaumeGomez deleted the wordcount-core branch June 14, 2025 15:54
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