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

sighandler() should take 2 arguments #2180

Closed
wants to merge 2 commits into from
Closed

sighandler() should take 2 arguments #2180

wants to merge 2 commits into from

Conversation

thegreatgazoo
Copy link

Stopping arcstat.py with ^C always ends up with error:
TypeError: sighandler() takes no arguments (2 given)

Signed-off-by: Isaac Huang he.huang@intel.com
Issue #2179

Stopping arcstat.py with ^C always ends up with error:
TypeError: sighandler() takes no arguments (2 given)

Signed-off-by: Isaac Huang <he.huang@intel.com>
Issue #2179
Now arcstat.py prints one header every hdr_intr
(20 by default) lines. It'd be nice if hdr_intr
is set according to terminal window size, i.e.
one header per screenful of outputs.

Signed-off-by: Isaac Huang <he.huang@intel.com>
Issue #2181
@behlendorf
Copy link
Contributor

@aarcane @prakashsurya could you please review these proposed changes.

@behlendorf behlendorf added the Bug label Mar 11, 2014
@behlendorf behlendorf added this to the 0.6.3 milestone Mar 11, 2014
@prometheanfire
Copy link
Contributor

why is there so much more (the new function and stuff)?

@ryao
Copy link
Contributor

ryao commented Mar 11, 2014

@prometheanfire @thegreatgazoo sent two patches, but made the pull request appear to only have one. I will not comment on the specifics of the code since I am not fluent in python.

@behlendorf
Copy link
Contributor

@prometheanfire It determines the total number of lines in your terminal so that value may be used as the default instead of 20. That makes the behavior consistent with tools like vmstat.

@thegreatgazoo
Copy link
Author

@prometheanfire when I was trying to make a pull request for the 2nd commit, it automatically got into this pull request. I'm still new to github so I must have made some mistake.

@prometheanfire
Copy link
Contributor

@thegreatgazoo github tracks the branch you do the pull request from (looks like your master branch). I would create a branch, for each feature you want to submit and then submit a pull request for each branch. In this case one to fix the bug and one to add support for the new function.

@thegreatgazoo
Copy link
Author

New pull requests sent: #2182 and #2183

@behlendorf
Copy link
Contributor

@thegreatgazoo Thanks for splitting this up. Just for future reference the only reasons you might want to include multiple patches in a single pull requests are:

  1. All the patches are related to a single new feature being added. For example, there is a stack of patches waiting to merged for the new event daemon infrastructure which are all related and in a single pull request.

  2. There's a dependency between patches.

@prometheanfire
Copy link
Contributor

also to keep in mind, a patch should be able to be applied atomicly without breaking things.

patch 1 of a 5 part patch series should be able to be applied alone without breaking things (maybe not only patch 2, since there may be a dep on patch 1 from patch 2, but you get the idea I hope).

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.

5 participants