-
Notifications
You must be signed in to change notification settings - Fork 11
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
Refactor to move methods to where they belong #119
Conversation
Test coverage isn't accurate since we have major part of our code in C extension Also, we are running each spec in a separate executable so coveralls doesn't show the right report.
this changeset looks good to me. |
@@ -106,15 +31,15 @@ def self.start_server(pub_port: DEFAULT_PUB_PORT, request_port: DEFAULT_REQ_PORT | |||
# profiling is not feasible. | |||
def self.start_profiling(pub_port: DEFAULT_PUB_PORT, request_port: DEFAULT_REQ_PORT, |
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.
i have a question, not entirely related to this PR.
why does start_profiling
return an instance of server? what is the use of returning an instance of server?
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.
Previously, you could do things like Rbkit.status
or Rbkit.send_objectspace_dump
. These methods don't really make sense because there's no indication that a server has been started to send the status or object space dump. I think it makes more sense to put these methods on an instance of Rbkit::Server, so it would be:
server = Rbkit.start_profiling # or Rbkit.start_server
server.status
server.send_objectspace_dump
server.stop # or Rbkit.stop_server
I don't expect users to use these APIs since they'll be using the GUI to send messages that trigger these methods, still it provides a way to do it from code. We are using this a lot inside our tests.
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.
okie, got it
Refactor to move methods to where they belong
Changes :
rbkit_tracer.c
torbkit_server.c
because the file contains not only code for tracing, but all other C functions in Rbkit server.Rbkit::Profiler
toRbkit::Server
because the methods inside the class were responsible for starting/stopping the server and processing commands from the client.Rbkit
module toRbkit::Server
class as instance methods. I think this move makes the code easier to reason about, and keeps the user's API (Rbkit module methods) minimal and less confusing. This will also contain all new commands inside theServer
class which will help implement Implement support for enabling and disabling of live object creation events #106 in a cleaner way