Skip to content

Conversation

@mrdziuban
Copy link
Contributor

@mrdziuban mrdziuban commented Oct 17, 2025

Related to #12 (comment)

Adds a new minify_css option. When set to true, style attributes will not contain trailing semicolons or spaces between style properties and values, e.g.

<!-- set to false (the default) -->
<h1 style="color: red;font-weight: bold;">Hello</h1>
<!-- set to true -->
<h1 style="color:red;font-weight:bold">Hello</h1>

Unrelated to the new option, I also made the following changes:

  1. Updated CI to run cargo fmt --all -- --check on the Java bindings
  2. Updated CI to run cargo clippy -- -D warnings on the Java bindings
  3. Added a testRuntimeOnly config to the Java gradle build to fix the error seen here
    Could not start Gradle Test Executor 1: Failed to load JUnit Platform.  Please ensure that all JUnit Platform dependencies are available on the test's runtime classpath, including the JUnit Platform launcher.
    
  4. Updates the Python bindings to add backticks to documentation to fix the Clippy errors seen here

There are some outstanding issues/questions:

  • I had to update the ruby build for the same reason described in feat: Add keep_at_rules option #485 (comment) -- is this correct?
  • The MSRV build fails with an error about rayon-core. This is also present in the latest master build, but let me know if/how I can resolve it
  • Should keepAtRules be added to bindings/javascript/js-binding.d.ts? See comment below
  • Should keep_at_rules be added to bindings/python/css_inline.pyi? See comment below

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 17, 2025

CodSpeed Performance Report

Merging #559 will not alter performance

Comparing mrdziuban:minify-css (6f5b6d9) with master (e189022)

Summary

✅ 6 untouched

@codecov
Copy link

codecov bot commented Oct 17, 2025

Codecov Report

❌ Patch coverage is 95.55556% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.25%. Comparing base (e189022) to head (6f5b6d9).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
css-inline/src/html/serializer.rs 94.11% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #559      +/-   ##
==========================================
+ Coverage   89.11%   89.25%   +0.14%     
==========================================
  Files          18       18              
  Lines        2048     2084      +36     
==========================================
+ Hits         1825     1860      +35     
- Misses        223      224       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.


[dependencies.css-inline]
version = "0.17"
path = "../../../../css-inline"
Copy link
Owner

Choose a reason for hiding this comment

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

This one is not properly documented, but it is the way to make an installable gem. There was a comment somewhere in closed issues (not sure if you mentioned that one on the original message, I’ll take a closer look when I am back to my pc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was doing my best to follow the conversation starting in #485 (comment), but I'm not sure if I did the right thing 😅

Copy link
Owner

Choose a reason for hiding this comment

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

Here is a more detailed explanation by Magnus' author. Briefly, the referenced path won't be packaged with the published crate (in contrast with PyO3 & related build system that packages everything needed), and during the installation time, cargo won't find it.

Right now, it is fine to have it like this. After releasing the Rust crate, I'll make the needed changes and will release the Ruby side separately

@Stranger6667
Copy link
Owner

Outstanding work! Thank you so much :)

Re: MSRV - let's bump it to 1.80

) -> Result<(), InlineError> {
writer.write_all(name.as_bytes())?;
writer.write_all(STYLE_SEPARATOR)?;
if minify_css {
Copy link
Owner

Choose a reason for hiding this comment

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

perf: I'd like to acknowledge that this branch affects performance a bit. I am fine with keeping it as is, as the impact is minimal. But at some point, I'd like to have generics that will abstract this behavior and allow for a monomorphized version without branching (if it won't affect the binary size / compile time much)

@mrdziuban
Copy link
Contributor Author

Thanks for reviewing! Bumped MSRV to 1.80.

@Stranger6667 Stranger6667 merged commit c3ee65d into Stranger6667:master Oct 22, 2025
65 of 76 checks passed
@Stranger6667
Copy link
Owner

Thank you so much! I'll check the failing builds and will proceed with a new release next weekend :)

@mrdziuban mrdziuban deleted the minify-css branch October 22, 2025 19:46
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