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

Replace missing forked ronn-ng with public nronn #841

Merged
merged 1 commit into from
Jan 1, 2023
Merged

Conversation

tnir
Copy link
Collaborator

@tnir tnir commented Aug 1, 2022

Use nronn to unify use of rdiscount with kramdown, and to unify use of hpricot with nokogiri.

  1. rdiscount and kramdown both provide similar functionality: avoid to use rdicount
  2. hpricot and nokogiri both provide similar functionality: avoid to use hpricot
  3. Tested with Ruby 3.1 in CI (ronn has never continuously tested 😭).

Signed-off-by: Takuya Noguchi [email protected]

@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-use-n-8bmmln August 1, 2022 11:36 Inactive
@tnir tnir marked this pull request as draft August 1, 2022 11:38
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-use-n-epslp9 August 1, 2022 11:39 Inactive
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-use-n-gepw7p August 1, 2022 23:26 Inactive
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-use-n-qakavd August 2, 2022 03:40 Inactive
@tnir tnir marked this pull request as ready for review August 2, 2022 03:45
Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Awesome find, this simplifies a lot.

@tnir tnir requested a review from deivid-rodriguez August 2, 2022 06:13
@deivid-rodriguez
Copy link
Member

I think it's a bit unfair to not mention ronn-ng at all here, since it's ronn-ng which took care of removing this dependency duplication. I think you should try to get ronn-ng mantainer to hand over maintenance to you, to save the whole community yet another migration to a different gem (I think OS packagers are already using ronn-ng).

If you don't succeed on taking over maintenance, then I think you should at least credit ronn-ng in this PR and in nronn's README. Also, I'm not sure why you didn't create nronn as a fork of ronn-ng, since that's what it is.

Copy link
Collaborator Author

@tnir tnir left a comment

Choose a reason for hiding this comment

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

Found a regression, pushing back to draft.

@tnir tnir marked this pull request as draft August 2, 2022 07:49
@deivid-rodriguez
Copy link
Member

We're getting segfaults with ronn-ng now for some reason. Maybe nronn fixes them? What was the regression that you found, @tnir? The new fork looked pretty great. I think as long as we give proper attribution to ronn-ng, we could move forward with it!

@tnir
Copy link
Collaborator Author

tnir commented Dec 25, 2022

@deivid-rodriguez Thanks. Were you looking at this failure only in GitHub Actions environment? I was not able to find failures even with ronn-ng on my local.

@tnir tnir self-assigned this Dec 25, 2022
@tnir tnir force-pushed the tnir-use-nronn branch 2 times, most recently from 1238820 to c1deb9f Compare December 25, 2022 21:59
@tnir
Copy link
Collaborator Author

tnir commented Dec 25, 2022

Anyway merge conflicts are now all resolved.

@deivid-rodriguez
Copy link
Member

@tnir Yes, I was looking at those failures indeed. Sometimes it does work, but they seem more frequent now. Since the crash happens deep inside hpricot's gem code, I figure moving away from that gem would do the trick.

Hpricot hasn't received updates for almost 10 years and declares itself as dead in its README, recommending to move on to nokogiri like nronn does.

I had a look at the site built from this PR and everything looks fine!

@tnir tnir marked this pull request as ready for review December 26, 2022 11:37
@deivid-rodriguez
Copy link
Member

@tnir Would you be willing to take over ronn-ng maintenance? If current maintainer is ok with that, that'd be probably best for the community.

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Dec 26, 2022

We can ask the current maintainer and see what he thinks. I don't think that needs to block this PR though, we could ask and merge this PR, and then switch back to ronn-ng in the future if the answer is positive.

Gemfile.lock Outdated Show resolved Hide resolved
@tnir tnir force-pushed the tnir-use-nronn branch 2 times, most recently from be83564 to ac40ae0 Compare December 26, 2022 21:59
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

@tnir Approving this since I don't think it should block this PR, but if you can confirm that you'd be willing to take over ronn-ng maintenance, I can ask upstream.

@deivid-rodriguez
Copy link
Member

Actually I'm doubly confused now that I double checked dependencies, since ronn-ng has also dropped the dependency on both nokogiri and rdiscount. What's the difference between nronn and ronn-ng?

@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-use-n-zbe5ww December 29, 2022 08:41 Inactive
To unify use of rdiscount with kramdown, and to unify use of hpricot
with nokogiri.

Signed-off-by: Takuya Noguchi <[email protected]>
@deivid-rodriguez deivid-rodriguez temporarily deployed to bundler-site-tnir-use-n-zbe5ww January 1, 2023 01:52 Inactive
@tnir tnir changed the title Unify duplicated use of rdiscount/kramdown and hpricot/nokogiri Replace missing forked ronn-ng with public nronn Jan 1, 2023
@tnir tnir added ruby Pull requests that update Ruby code regression labels Jan 1, 2023
@tnir tnir merged commit 99054ef into master Jan 1, 2023
@tnir tnir deleted the tnir-use-nronn branch January 1, 2023 01:56
@deivid-rodriguez deivid-rodriguez mentioned this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to pull deivid-rodriguez/ronn-ng@fix-charset
3 participants