Added timestamps in crontab.py(#928)

[anweber111]

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #928

Status

READY | UNDER DEVELOPMENT | ON HOLD

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update
• Documentation (fix or adds documentation)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

• Test A
• Test B

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[FabioRosado]

Thank you for working on this issue, it seems that all tests are passing but linting. time needs to be the first thing imported, so if you could update this PR to fix this linting issue I will be happy to merge it into core 👍

[codecov]

Codecov Report

Merging #930 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #930   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          37     37           
  Lines        2342   2343    +1     
=====================================
+ Hits         2342   2343    +1

Impacted Files	Coverage Δ
opsdroid/parsers/crontab.py	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ed0eee...5cd865b. Read the comment docs.

[anweber111]

@FabioRosado done that.

[jacobtomlinson]

Thanks for doing this! Have a couple of questions.

[jacobtomlinson]

Welcome to open source, it's awesome to have you here 🎉! This kind of code review is pretty standard for open source contribution. Please don't be put off by the questioning, it's not a criticism, I'm just keen to understand why you have made these decisions.

Particularly I'm keen to understand the difference between :

• time.asctime(time.localtime(time.time()))
• time.asctime(time.localtime())
• time.asctime()

We should aim to add the least code possible to meet the requirement. Do the nested calls add value? I can imagine that localtime might help us with timezones.

As for the original message, that's probably my fault for not being clear in the issue. I would like to keep the original message and also add the time to it.

[anweber111]

@jacobtomlinson you are right I should probably not simply have done what I always do with time, thank you for the correction. Anyways, after reading in the API I have committed and pushed a new version - I do not think we need the format, though - at least when I try it it does not change the result.

[FabioRosado]

I believe Jacob was just asking if there was a difference between the different ways to show up time, but since you tested it out and there was no different from the output I'd say this way is much easier to read 😄

Using format on the logger would have caused a linting failure since I did that on my previous PR and it failed haha

I am not sure if you tried this but if you change the Logging line from:

_LOGGER.debug(_("Running crontab skills at " + time.asctime()))

to

_LOGGER.debug(_("Running crontab skills at %s ", time.asctime()))

Will it work exactly the same as well? I'm asking this since we have been using the above way to format the logging throughout the codebase.

Also, the test that failed doesn't have anything to do with your code so don't worry about it since the changes that you did with this PR are all green and passing 😄 👍

[anweber111]

@FabioRosado I did just now test your other suggestion - to be honest I used a print for testing but this should not matter in this case - for some reason, if I use your suggestion I get:

Running crontab skills at %s Tue May 14 09:04:34 2019

So - I really wonder, I guess you do not really want the %s in the text - why does this not happen in the rest of the logging? Or does it?

Uhm..........can you answer my pull request? I know it seems childish but I am kinda excited to be contributor to some project.

[FabioRosado]

I am going to assume that the issue is how the print is working, since I just tested it on my end and this is what I get: opsdroid> DEBUG opsdroid.parsers.crontab: Running crontab skills Tue May 14 08:16:00 2019

But I've also noticed an issue with my suggestion, that line needs to be: _LOGGER.debug(_("Running crontab skills at %s "), time.asctime())

and perhaps add a separator between the end of the sentence and the %s something like Running crontab skills at - %s .

It's understandable that you want to get your first PR merged, this looks really good and we are very happy for you to take the time in contributing to the project 😄 👍

We need to try and fix appveyor and then we are able to merge this PR into core 👍

[jacobtomlinson]

I agree with @FabioRosado that it should be _LOGGER.debug(_("Running crontab skills at %s "), time.asctime()). The reason we want to keep the %s in the string is because different languages may place the time differently within the sentence, so we should use the flexibility of the formatting rather than concatenating the strings.

I don't think the AppVeyor failures are related to this,.

[jacobtomlinson]

Here is a change suggestion based on @FabioRosado's suggestion. I'm just going to suggest you make this change so we can get this over the line.

[FabioRosado]

@anweber111 thank you for updating this PR again. Jacob has fixed the issue we where getting on appveyor so as soon as the tests pass after I updated this branch we are ready to have this PR merged into the core.

This is amazing work and will help us when trying to debug things so thank you again for working on this 👍

Also, we send stickers to new contributors so feel free to send your home address by DM to opsdroid twitter account. I hope you enjoyed your first contribution and hope to see you again in this Project 😄