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

3.9.x: Support i18n 1.x #9269

Merged
merged 1 commit into from
Jan 25, 2023
Merged

3.9.x: Support i18n 1.x #9269

merged 1 commit into from
Jan 25, 2023

Conversation

parkr
Copy link
Member

@parkr parkr commented Jan 25, 2023

This is a 🙋 feature or enhancement.

The test suite passes locally (run script/cibuild to verify this)

Summary

The i18n plugin was added in 93e3eb06d2 to allow for some extra options to the slugify filter. At the time, we locked it to i18n v0.x, not allowing v1.0. This is a reasonable solution since we didn't know what v1 would hold. v1 is out (and has been out a while) and it's now required by the newest version of activesupport, a common library used by/with Jekyll. We should update v3.x to allow compatibility with this newer gem version.

Context

Closes #9268.

/cc github/pages-gem#866

@parkr parkr requested a review from a team January 25, 2023 03:33
@parkr
Copy link
Member Author

parkr commented Jan 25, 2023

Cucumber and Minitest pass on my machine.

@parkr
Copy link
Member Author

parkr commented Jan 25, 2023

Hm, tests seem to fail due to 1 rdiscount failure. Maybe it's related to the i18n version? Hm. Also, I can't believe we still test rdiscount – we don't waste our energy on rdiscount in Jekyll 4, do we?

Failure:
TestRdiscount#test_: rdiscount should render toc.  [/home/runner/work/jekyll/jekyll/test/test_rdiscount.rb:47]
Minitest::Assertion:
--- expected
+++ actual
@@ -1,13 +1,13 @@
-"<a name=\"Header.1\"></a>
+"<a name=\"Header-1\"></a>
 <h1>Header 1</h1>
 
-<a name=\"Header.2\"></a>
+<a name=\"Header-2\"></a>
 <h2>Header 2</h2>
 
 <p><ul>
- <li><a href=\"#Header.1\">Header 1</a>
+ <li><a href=\"#Header-1\">Header 1</a>
  <ul>
-  <li><a href=\"#Header.2\">Header 2</a></li>
+  <li><a href=\"#Header-2\">Header 2</a></li>
  </ul>
  </li>
 </ul>

@parkr
Copy link
Member Author

parkr commented Jan 25, 2023

Rdiscount failure also occurred here: #9272

So it must be a breaking change. We should just update our test.

@ashmaroli ashmaroli added the Jekyll 3.x Updates for legacy Jekyll 3.x series label Jan 25, 2023
@ashmaroli
Copy link
Member

ashmaroli commented Jan 25, 2023

So it must be a breaking change. We should just update our test.

gem rdiscount only bumped from 2.2.0.2 to 2.2.7 to mirror underlying native extension discount which bumped from 2.2.0 to 2.2.7. So, assuming they follow Semantic Versioning, our test was depending on a buggy behavior.

I recommend updating the test (or locking to last known compatible version) via a separate pull request (or a direct commit to the 3.9-stable branch).

I can't believe we still test rdiscount – we don't waste our energy on rdiscount in Jekyll 4, do we?

Jekyll 4 dropped built-in support for rdiscount and redcarpet. So, related tests were removed as well.
The 3.x-stable branches still tests these Markdown processors.

@parkr parkr merged commit fe12951 into 3.9-stable Jan 25, 2023
@parkr parkr deleted the 3.9-stable-i18n branch January 25, 2023 23:53
@jekyll jekyll locked and limited conversation to collaborators Jan 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Jekyll 3.x Updates for legacy Jekyll 3.x series
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants