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

Updater: Log info message when receiving signal #951

Merged
merged 5 commits into from Jan 9, 2018

Conversation

Projects
None yet
5 participants
@Makman2
Copy link
Contributor

Makman2 commented Dec 25, 2017

Closes #946

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 25, 2017

Codecov Report

Merging #951 into master will decrease coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #951      +/-   ##
==========================================
- Coverage    91.8%   91.69%   -0.12%     
==========================================
  Files         103      103              
  Lines        4040     4046       +6     
  Branches      638      639       +1     
==========================================
+ Hits         3709     3710       +1     
- Misses        193      197       +4     
- Partials      138      139       +1
Impacted Files Coverage Δ
telegram/utils/helpers.py 82.35% <100%> (+2.35%) ⬆️
telegram/ext/updater.py 77.17% <100%> (+0.25%) ⬆️
telegram/utils/request.py 66.96% <0%> (-0.9%) ⬇️
telegram/bot.py 87.51% <0%> (-0.5%) ⬇️
@Makman2

This comment has been minimized.

Copy link
Contributor Author

Makman2 commented Dec 25, 2017

Wondering why coverage decreased, haven't introduced any new branches...

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Dec 25, 2017

@Makman2 Sometimes coverage fluctuates because errors do or don't happen. The important part is that the diff is 100% covered. I'll look more detailed after Christmas. Thanks for contributing.

@robert-cody

This comment has been minimized.

Copy link

robert-cody commented Dec 25, 2017

def signal_name_by_code(code):
    return next((n for n in dir(signal) if n.startswith("SIG") and "_" not in n and getattr(signal, n) == code))

Looks better.

@Makman2

This comment has been minimized.

Copy link
Contributor Author

Makman2 commented Dec 26, 2017

Okay that shifts lookup effort to each call instead of once preparing a dict inside the module. But we shouldn't raise an implicit StopIteration in case there's no such signal-code^^ Might have side-effects when used in for-loops (in case that happens). At least we should do this imo:

try:
    return next(...)
except StopIteration:
    raise KeyError('Unknown signal number {}'.format(...))

May I call it signal_name_from_number though? by sounds less descriptive for me + python docs don't mention the term "code" for those signal numbers^^

@robert-cody

This comment has been minimized.

Copy link

robert-cody commented Dec 26, 2017

@Makman2 you're right about StopIteration. You can name your method as you wish, i'm not this repo developer, just proposed less code solution.

@jsmnbom

This comment has been minimized.

Copy link
Member

jsmnbom commented Dec 30, 2017

We in the dev team are hesitant about adding new dependencies (even just for tests). Especially since in this case it doesn't seem to be required as log can already be captured using pure pytest. See Loggging - pytest documentation for details.
Could you maybe update the code so it uses that style instead of the extra dependency?


# From https://stackoverflow.com/questions/2549939/get-signal-names-from-numbers-in-python
_signames = {v: k
for k, v in reversed(sorted(vars(signal).items()))

This comment has been minimized.

@tsnoam

tsnoam Dec 30, 2017

Member

nitpicking here. the reversed & sorted are useless here as you're creating a dict. please remove them.

This comment has been minimized.

@Makman2

Makman2 Dec 30, 2017

Author Contributor

I think the sorted was there for signals with different names but same number, so there's a deterministic outcome. However I haven't understood the reversed call too^^

This comment has been minimized.

@Makman2

Makman2 Dec 30, 2017

Author Contributor

And kk reversed then picks signal names that are sorted at the beginning. On my system I get these differences when I don't use reversed:

SIGIOT vs SIGABRT
SIGCLD vs SIGCHLD
SIGPOLL vs SIGIO

Though not sure if it's that important here^^

This comment has been minimized.

@tsnoam

tsnoam Jan 7, 2018

Member

Check manpage of signal(7) all the above are synonyms. If one name is chosen over another it's merely a matter of luck, as dictionary order is not guaranteed.
On my system I had different results on different executions.

Another option is to leave the signal name alone and just print the number.

This comment has been minimized.

@Makman2

Makman2 Jan 7, 2018

Author Contributor

If one name is chosen over another it's merely a matter of luck, as dictionary order is not guaranteed.

Looks like it's true, I'm just wondering: the order we iterate over should always produce a guaranteed outcome. vars(signal).items() has unique mappings from name -> number. Using sorted we get always the same list with the same order, and then we are filling the dict with those values, while switching name and number. If there are numbers that are equal, later elements in the list should overwrite the old ones. Although using dict-comprehension, citing the dict-docs:

If a key occurs more than once, the last value for that key becomes the corresponding value in the new dictionary.

This comment has been minimized.

@Makman2

Makman2 Jan 7, 2018

Author Contributor

Another option is to leave the signal name alone and just print the number.

I wouldn't do that, signal numbers differ on OSes for the "same" signals. I think people identify those signals usually by their name, like SIGINT / SIGTERM.

This comment has been minimized.

@tsnoam

tsnoam Jan 9, 2018

Member

ok. not worth the argument, lets keep it as is.

@Makman2

This comment has been minimized.

Copy link
Contributor Author

Makman2 commented Dec 30, 2017

We in the dev team are hesitant about adding new dependencies (even just for tests). Especially since in this case it doesn't seem to be required as log can already be captured using pure pytest. See Loggging - pytest documentation for details.
Could you maybe update the code so it uses that style instead of the extra dependency?

Sure no problem 👍

@Makman2

This comment has been minimized.

Copy link
Contributor Author

Makman2 commented Jan 1, 2018

About

We in the dev team are hesitant about adding new dependencies (even just for tests). Especially since in this case it doesn't seem to be required as log can already be captured using pure pytest. See Loggging - pytest documentation for details.
Could you maybe update the code so it uses that style instead of the extra dependency?

I have tried using this feature, however:

  • It requires the installation of pytest-capturelog, so a new package is anyway needed for this
  • Equivalent testing code with that package fails, pytest gets terminated too early:
    tests/test_updater.py FTerminated
    

So I believe using testfixtures is currently the easiest solution.

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Jan 4, 2018

@Makman2
We prefer using pytest ecosystem if possible (so if it comes down to adding a new dependency the prefernce will be for it).
Can you please commit your code for testing with pytest to your branch and allow edits for maintainers as described here:
https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

we'll try to work on it together before using testfixtures.

Use pytest-capturelog instead of testfixtures
This makes tests fail though...
@Makman2

This comment has been minimized.

Copy link
Contributor Author

Makman2 commented Jan 4, 2018

Done 👍

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Jan 7, 2018

The problem with pytest in the test you've sent is that it catches the signal itself / prevents Updater from catching it.
I assume that testfixtures forks and runs the test in a different process, thus ignoring the issue.
I'll see what can be done here.

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Jan 7, 2018

Note to self: need to make sure that this PR works properly on windows as the unitest in question is skipped on appveyor.

tsnoam added some commits Jan 7, 2018

Fix test_idle() with caplog
TODO: Fix warning about deprecated fixture declaration
@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Jan 7, 2018

@Makman2 - I believe that I fixed the relevant test. It was a matter of race condition (the signal arrived too soon).

@Eldinnie - Can you please (manually) verify on windows that the new code doesn't break anything?

'telegram.ext.updater', 'INFO', 'Received signal {} (SIGTERM), '
'stopping...'.format(signal.SIGTERM))]
rec = caplog.records()[-1]
assert rec.msg.startswith('Received signal {}'.format(signal.SIGTERM))

This comment has been minimized.

@Makman2

Makman2 Jan 7, 2018

Author Contributor

just curious, why not asserting for the complete message? :)

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Jan 8, 2018

> python .\examples\echobot2.py
2018-01-08 15:10:03,051 - telegram.ext.updater - INFO - Received signal 2 (SIGINT), stopping...
>

Works fine on windows.

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Jan 9, 2018

Build fails due to unrelated problems fixed in a different PR.
Will merge now.

@Makman2 Thanks for your contribution.

@tsnoam tsnoam merged commit eb67c03 into python-telegram-bot:master Jan 9, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate All good!
Details

@Makman2 Makman2 deleted the Makman2:Makman2/logexit branch Jan 9, 2018

@Makman2

This comment has been minimized.

Copy link
Contributor Author

Makman2 commented Jan 9, 2018

Thanks for helping me out and bearing with me ;D

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Jan 9, 2018

@Makman2 All the thanks are for you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.