Feature logger #21
Feature logger #21
Conversation
|
Here's the code health analysis summary for commits Analysis Summary
|
|
Git asking to add to name of "LogMessage" method, because because it sees Task in the method return parameter, but I use |
There seems to be a lot of debate in the c# community regarding the naming of async functions. I chose to go with Microsoft's guidelines, and add the Async suffix to the method names, as DeepSource tells me to do. The internet has different opinions about adding the Async suffix to Tasks without the async modifier aswell. I'd rename it to LogMessageAsync, but I'm not sure how others like it. I'm not really sure what to do here to be honest |
In this case it is not necessary, since the method does not perform asynchronous operations. |
In this case, the issue can be ignored by adding a comment in the same line, or the line above: |
It was good idea :) |
|
Sorry for the delay, I'm going to try to review the pr today and suggest some changes if needed |
|
You don't merge PR. |
Sorry, I've been out of town for a few days, so I wasn't able to review it I should be back in a few days, and I'll try to review it when I can Sorry for the delay |
There was a problem hiding this comment.
I meant the namespace in the file, not the folder name, sorry :/
So the folder would be called Logger (and the file path would be /Logger/SerilogConfiguration.cs), while the namespace on line 9 could be for example QuickEdit.Logger
No problem. Glad to have you back! |
|
I renamed the main folder to "Logger" and deleted the "LoggerImplementation" folder. I hope we understand each other correctly now, if not, please let me know. |
There was a problem hiding this comment.
Looks good!
I'd clean up the commit history a bit by doing an interactive rebase if you're ok with that. It would basically rewrite the commit history, in such a way that, for example, some of the small commits fixing an issue with the previous commit, which are quite common when working on a feature, could be merged into one, cleaning up the history a bit before getting merged.
This would also require a force push, to rewrite the commits. I always make sure to keep a backup of the branch, though, in case something goes wrong.
After that, I would slightly clean up all the files, like for example fixing some broken indentations in some places probably caused by me forgetting to add them in the review suggestions :P
I'm not an expert at git, and this is pretty much my first time collaborating with other people on GitHub (thanks :)), which is why I'm always asking about everything. I can just leave the commit history as is and merge the PR, or squash merge it if needed, but I just thought it would be better to tidy it up a bit.
Yes, indeed, the PR looks dirty, so I'm all for you cleaning it up. I'm also far from being a professional in git and this is also my first work in collaboration (thank you very much too!), but as far as I understand, I can't help with cleaning the commits, since I don't have the necessary rights in your repository. However, I can clean the code and upload the commit. After which you will start cleaning, okay? |
Yeah, you can do that if you want. I can do it if you don't want to, and as for the history clean-up, I wanted to do it in this pull request, so on your fork, in your branch, as I think I should have the necessary permissions for it, but I'm not sure yet. After that, I'll merge the pr |
Ok, I didn't know it was so easy to clean up code from extra spaces and unused directives. In that case, I'm waiting for your code cleaning, as well as cleaning up commits. |
Fix DeepSource flagging `AutoLog.LogMessage()` as async (CS-R1073)
Co-authored-by: Jakub Dębski <90698026+HEJOK254@users.noreply.github.com>
Improved log messages by: - Adding more error information to the log messages - Making them less verbose - Adding additional debug information in debug mode (config.json) Co-authored-by: Jakub Dębski <90698026+HEJOK254@users.noreply.github.com>
Co-authored-by: Jakub Dębski <90698026+HEJOK254@users.noreply.github.com>
Fixed things like: - indentation - formatting - file format (Central European (Windows) -> UTF-8) - access modifiers (program class is now internal) - removed unused `using` directives
|
I pushed the changes to HEJOK254/Discord-QuickEdit/feature/logger-preview, instead of force pushing it to your branch, so we can discuss the changes and change things if needed, without having to force push on your branch all the time. Please check if everything looks ok, or if I should rename/delete/squash etc. some commits. I tried to keep the commits small, which should help in troubleshooting in case one of them introduced some issue, while also grouping together changes regarding the same "feature", like for example improving the log messages. If you have any suggestions, let me know, and I'll be happy to change it |
I looked at the commits in this branch. They look clean and structured. I have no ideas/wishes, in my opinion you did everything well. |
HEJOK254
force-pushed
the
feature-logger
branch
from
657be7a to
a5dd282
Compare
- Removed log enrichment - Changed the path to the log file to the universal directory Co-authored-by: Jakub Dębski <90698026+HEJOK254@users.noreply.github.com>
Since 76c6a1 the package `Serilog.Enrichers.Environment` is not needed anymore, as it's unused Forgot to remove it while reviewing the PR whoops
Empty interpolated string (CS-W1000) Forgot to remove that `$` while reviewing the PR whoops (76c6a12)
|
I forgot to remove that Anyway, I think the PR is ready to be merged. If you have any questions or any more changes you would like to make, let me know, if not, I'll merge it soon :) |
No questions or suggestions. Thank you for working together! |
Ok, I'll merge it now Thanks for helping me with the project :) |
No description provided.