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

Make rinja #![no_std] compatible #286

Merged
merged 4 commits into from
Dec 21, 2024
Merged

Conversation

Kijewski
Copy link
Collaborator

@Kijewski Kijewski commented Dec 13, 2024

Resolves #15.

  • add tests for a full no-std environment
  • add tests for an alloc-only environment

The change is only so big because I split up the built-in filter implementation into two files: full no-std, and with alloc.

@Kijewski Kijewski changed the title Pr no std Make rinja #![no_std] compatible Dec 13, 2024
@Kijewski Kijewski force-pushed the pr-no-std branch 6 times, most recently from 54cc1a9 to 95ee42d Compare December 16, 2024 08:55
@Kijewski Kijewski marked this pull request as ready for review December 16, 2024 08:55
percent-encoding = { version = "2.1.0", optional = true }
serde = { version = "1.0", optional = true }
serde_json = { version = "1.0", optional = true }
percent-encoding = { version = "2.1.0", optional = true, default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

One less dependency, yeay! \o/

/// Compare with [fmt](./fn.fmt.html).
pub fn format() {}

/// Replaces line breaks in plain text with appropriate HTML
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a dot at the end of this sentence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I kept the comments as they are in the current master, I just split the file in two. I think it's easier to follow the individual commits than looking at the whole diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll make a typo pass later on then. ;)

Ok(HtmlSafeOutput(linebreaks(try_to_str!(s => buffer))))
}

/// Converts all newlines in a piece of plain text to HTML line breaks
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too.

Ok(HtmlSafeOutput(linebreaksbr(try_to_str!(s => buffer))))
}

/// Replaces only paragraph breaks in plain text with appropriate HTML
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm seeing a repeated behaviour here. 🤣

Well, I suppose it's not too important so stopping comments for this from here.

Ok(output)
}

#[cfg(all(test, feature = "alloc"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can just be:

Suggested change
#[cfg(all(test, feature = "alloc"))]
#[cfg(test)]

since this file is already imported only with #[cfg(feature = "alloc")].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yes. Replaced.

To make the next change more readable / easy to follow.
This leads to fewer needless allocations, and makes the operator `?`
usable in an no-alloc context.
@GuillaumeGomez
Copy link
Contributor

Looks good to me, thanks a lot! Just one thing I thought about: we should mention these feature in the book and the crate(s?) docs as well.

@Kijewski
Copy link
Collaborator Author

Yes, I guess it would be a good idea to add an explanation what our features do. To a more seasoned rust developer, the feature std and alloc might be self-explanatory, but urlencode and when I need serde_json might be not.

@Kijewski Kijewski merged commit ea4b2b3 into rinja-rs:master Dec 21, 2024
30 checks passed
@Kijewski Kijewski deleted the pr-no-std branch December 21, 2024 18:23
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.

Add no_std feature?
2 participants