Attempt at example for logging consistency for discussion #1221
Conversation
|
Would also be good to get eyes from @opsdroid/maintainers on this. |
There was a problem hiding this comment.
I've already given my feedback on the punctuation issue. I've also noticed something on the log messages when they are split (this was done before we used black).
Also we should keep in mind that whatever changes we do to the logs message we will need to update all the translations otherwise they will stop working.
Codecov Report
@@ Coverage Diff @@
## master #1221 +/- ##
======================================
Coverage 100% 100%
======================================
Files 51 51
Lines 2775 2775
======================================
Hits 2775 2775
Continue to review full report at Codecov.
|
|
Thoughts on the shift here @opsdroid/maintainers? |
There was a problem hiding this comment.
Hello @iguyking sorry for taking so long to check this PR. I've added a few comments and suggestions (you can easily just click the button to commit these to your PR).
All the rest I'm pretty happy with it. We just need to update all the translated catalogs. Do you know how to do this or would you like me to explain?
|
@iguyking are you able to pick this up again? |
There was a problem hiding this comment.
This looks good. Thanks for all the effort here. I'm sure we could nitpick forever with this one so I'm keen to just get it merged. We can always continue to iterate on this later.
Some future steps we may consider:
- Ensure all translations still work while we've updated these strings
- Maybe add some tests to enforce the format?
|
If you need help with updating the translations you can check the documentation on localization. I haven't tried to update the translations yet but I think you can just run the command:
then compile them with the command: This should be enough, if you have any issue let me us know 😄 |
|
Looks like we've still got a failing test here that is looking for a specific log message (that now has a period at the end and doesn't match). Also black needs to be run. @iguyking could you fix this couple of issues so we get a green tick and can merge? |
Fix Co-Authored-By: Fábio Rosado <fabioglrosado@gmail.com>
Double Quotes Cleanup Co-Authored-By: Fábio Rosado <fabioglrosado@gmail.com>
|
@opsdroid/maintainers Fixed for the appveyor and travis-ci passing now. Couple fixes around a test case and cleaning up more logging along with associated documentation. Anything else? |
There was a problem hiding this comment.
Some code was removed/moved in another PR that has been merged. This PR seems to be reinstating the code (maybe due to a bad merge conflict resolution) and therefore the coverage is failing as the tests have also been removed.
@FabioRosado could you confirm that the sections I've commented on have been removed and should also be removed here?
(Sorry about this @iguyking, we are so close to merging!)
|
Yeah I can confirm that those bits where the ones I have either moved to configuration or removed it from the codebase |
|
Thanks @FabioRosado. I don't seem to be able to push onto this branch. @iguyking could you please delete the sections of code I highlighted and push again? |
|
Thanks fo much for your patience here @iguyking! Glad to finally get this in. |
An quick run of connectors with a shot at rules around logging consistency.
Description
A quick run against connectors tree with a shot at something that might be useful for documenting consistency in logging.
Fixes #1137
Status
UNDER DEVELOPMENT
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Basic documentation updates effectively. Should not be breaking type of changes unless there is some kind of width restriction concern for a given connector. Most of my changes reduced the general length of the messages, with a couple just adding a character or two.
Checklist: