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

Implement restart functionality #1333

Merged
merged 4 commits into from
Jan 31, 2019
Merged

Implement restart functionality #1333

merged 4 commits into from
Jan 31, 2019

Conversation

dgw
Copy link
Member

@dgw dgw commented May 17, 2018

This PR is an up-to-date version of the seemingly abandoned #1103 by @pachuco.

Personally, I need to dig more into why the pid_file parameter exists in run(), and why it was removed in this PR. But any further changes I might make to this pull request as a result will be in new commits, so as not to overwrite @pachuco's author credits in the commit log.

Assigning to 7.0.0 for now, to match the tentative (but never official) placement of the original PR. (Target milestone subject to change, of course.)

@dgw dgw added this to the 7.0.0 milestone May 24, 2018
@dgw
Copy link
Member Author

dgw commented May 28, 2018

Best I can tell, pid_file was added to run() for Reasons™ way back. As best I can tell from testing locally, it looks like the PID file is handled just fine with these changes, so I'll strike that note from the OP.

After looking into this, though, I think a --restart command-line option to go with --quit and --kill would be appropriate. I'll get to that (and am self-assigning the PR as a reminder), so for now I'll remove the "Needs Review" label because the PR's not really finished any more.

@dgw dgw self-assigned this May 28, 2018
@dgw
Copy link
Member Author

dgw commented May 28, 2018

Still WIP on the CLI front, but I did notice that bot.restart() didn't actually work and fixed that separately.

@dgw
Copy link
Member Author

dgw commented May 30, 2018

I'm done implementing. No unit tests, because I'm not really sure how to test restarting within the testing framework (it uses a mock Sopel, not a real process). Moving back to Needs Review.

Copy link
Contributor

@Hanicef Hanicef left a comment

Choose a reason for hiding this comment

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

Approved. It's also nice that you explained why SIGTERM and SIGILL are used instead of just SIGUSR*.

@dgw
Copy link
Member Author

dgw commented Jan 22, 2019

Rebased on master to resolve merge conflict.

@Exirel
Copy link
Contributor

Exirel commented Jan 31, 2019

I'm so sorry for the merge conflict all over the place. :D

sopel/run_script.py Outdated Show resolved Hide resolved
pachuco and others added 4 commits January 31, 2019 10:13
Adapted from 3b7dcad in PR 1103. I just removed some stuff that broke.
— dgw
"TypeError: 'bool' object is not callable" is all .restart would do,
because `restart` was defined as a class method and then overridden as a
boolean flag when the class was instantiated. Kept the restart() method
name, renamed the flag, Bob's your uncle.
A few commits back, the commit adding the "restart" command originally
tried to do some cleanup, too. It was left out of the final version of
PR #1333 due to breaking the use of ^C to quit Sopel interactively.

These TODO comments will serve as a reminder to eventually figure out
why the hell shit broke from something that should be simple, like
replacing os._exit() calls (not a great habit) with return values.
Something that shouldn't cause any change in function, but somehow did.
@dgw dgw merged commit f13baf9 into sopel-irc:master Jan 31, 2019
@dgw dgw deleted the 1103-restart-command branch January 31, 2019 16:17
kwaaak pushed a commit to kwaaak/sopel that referenced this pull request Mar 25, 2019
A few commits back, the commit adding the "restart" command originally
tried to do some cleanup, too. It was left out of the final version of
PR sopel-irc#1333 due to breaking the use of ^C to quit Sopel interactively.

These TODO comments will serve as a reminder to eventually figure out
why the hell shit broke from something that should be simple, like
replacing os._exit() calls (not a great habit) with return values.
Something that shouldn't cause any change in function, but somehow did.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants