Skip to content

Various fixes #37

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Various fixes #37

wants to merge 6 commits into from

Conversation

BuonOmo
Copy link
Collaborator

@BuonOmo BuonOmo commented Jan 22, 2025

Fixes #36

I've made a commit per fix, please tell me if you want separate PRs.

@BuonOmo BuonOmo force-pushed the patch-1 branch 3 times, most recently from 204b1f0 to 05c1be4 Compare January 22, 2025 10:11
@BuonOmo BuonOmo changed the title fix: rename Rack to Rackup Various fixes Jan 22, 2025
@@ -22,4 +22,4 @@ parser.parse!
server = StackProf::Webnav::Server
server.cmd_options = options

Rack::Handler.pick(['thin', 'webrick']).run server.new, :Host => options[:addr], :Port => options[:port]
Rackup::Handler.pick(['thin', 'webrick']).run server.new, :Host => options[:addr], :Port => options[:port]
Copy link
Owner

Choose a reason for hiding this comment

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

does this mean this won't work on old ruby versions?

@@ -45,7 +45,7 @@ def method_info name
:callers => callers(frame, info),
:callees => callees(frame, info),
:location => file,
:source => BetterErrors::CodeFormatter::HTML.new(file, line).output
:source => file && line ? BetterErrors::CodeFormatter::HTML.new(file, line).output : nil
Copy link
Owner

Choose a reason for hiding this comment

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

interesting in what scenario these are missing? maybe reproduce in unit test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could for sure, they're missing in C methods.

@alisnic
Copy link
Owner

alisnic commented Jan 30, 2025

I'm no longer working in Ruby ecosystem, so not sure i can properly review that. I left some comments, feel free to tackle them as you see fit. I also invited you to this repository as a collaborator. Once you accept the invite you will be able to merge/push by yourself.

Also, let me know what's your RubyGems email, and I'll figure out a way to give access to you to push the gem there

@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Jan 30, 2025

@alisnic fair enough, it's [email protected] but I'm currently trying to recover the account, so I can't push yet. I hope I'll figure it out soon.

The comments are quite relevant, I'll address all of them :)

- Update Ruby versions to 3.2, 3.3, and head
- Use `actions/checkout@v4` instead of `actions/checkout@v2`
- Cancel concurrent jobs
@BuonOmo BuonOmo force-pushed the patch-1 branch 4 times, most recently from ad24f5c to 53a0454 Compare February 10, 2025 07:53
spec.add_dependency "stackprof", ">= 0.2.13"
spec.add_dependency "better_errors", "~> 1.1.0"
spec.add_dependency "better_errors", "~> 2.10.1"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This actually fixes the cfunc bug. And closes #34

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.

Fails to run because of changes in Rack
2 participants