-
Notifications
You must be signed in to change notification settings - Fork 209
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
Conversation
There was a problem hiding this 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.
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. |
There was a problem hiding this 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.
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! |
@deivid-rodriguez Thanks. Were you looking at this failure only in GitHub Actions environment? I was not able to find failures even with
|
1238820
to
c1deb9f
Compare
Anyway merge conflicts are now all resolved. |
@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! |
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. |
be83564
to
ac40ae0
Compare
There was a problem hiding this 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.
Actually I'm doubly confused now that I double checked dependencies, since |
To unify use of rdiscount with kramdown, and to unify use of hpricot with nokogiri. Signed-off-by: Takuya Noguchi <[email protected]>
Use nronn to unify use of rdiscount with kramdown, and to unify use of hpricot with nokogiri.Signed-off-by: Takuya Noguchi [email protected]