Skip to content

Conversation

@ioquatix
Copy link
Contributor

@ioquatix ioquatix commented Mar 4, 2025

No description provided.

@ioquatix ioquatix marked this pull request as draft March 4, 2025 22:42
@ioquatix ioquatix marked this pull request as ready for review March 4, 2025 22:55
@ioquatix
Copy link
Contributor Author

ioquatix commented Mar 4, 2025

Okay this is now working for me locally.

Copy link
Owner

@bryanp bryanp left a comment

Choose a reason for hiding this comment

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

Thanks!

@bryanp
Copy link
Owner

bryanp commented Mar 4, 2025

@ioquatix do you mind addressing the linter failure? https://github.com/bryanp/llhttp/actions/runs/13664554845/job/38204178670?pr=32

@ioquatix
Copy link
Contributor Author

ioquatix commented Mar 4, 2025

Yes sure, those lines were me debugging the server tests. I've removed them.

@ioquatix
Copy link
Contributor Author

ioquatix commented Mar 4, 2025

This PR also changes the minimum Ruby version. Hopefully that is okay?

@bryanp
Copy link
Owner

bryanp commented Mar 4, 2025

Can it be dropped down to 3.0? This gem is used by http.rb which still depends on 3.0: https://github.com/httprb/http/blob/1af3c8e2dfb84d73e7febaaffce7bd7f4829cced/http.gemspec#L28

If that's an issue I can open a PR over there to try and bump it next release.

@ioquatix
Copy link
Contributor Author

ioquatix commented Mar 5, 2025

My suggestion would be to skip those tests on older Ruby versions, as there is nothing intrinsically problematic about the code in lib.

@bryanp
Copy link
Owner

bryanp commented Mar 5, 2025

The issue is that you bumped the minimum ruby version in the gemspec to 3.1—doesn't that make new releases of this gem incompatible with http.rb?

@ioquatix
Copy link
Contributor Author

ioquatix commented Mar 5, 2025

Yes, you'd need to revert the changes I made to the gemspec, and then only pull in async for the tests on Ruby 3.1+. Are you able to get this PR across the finish line?

@bryanp
Copy link
Owner

bryanp commented Mar 5, 2025

Yeah will do, thanks again for your help.

@bryanp bryanp force-pushed the update-async-tests branch from 864967f to 4992e19 Compare March 5, 2025 05:46
@bryanp bryanp mentioned this pull request Mar 5, 2025
@bryanp bryanp merged commit 184d214 into bryanp:main Mar 5, 2025
11 checks passed
@ioquatix ioquatix deleted the update-async-tests branch March 5, 2025 08:43
@ioquatix
Copy link
Contributor Author

ioquatix commented Mar 5, 2025

Great! Well done.

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