Feature logger

[deepsource-io[bot]]

Here's the code health analysis summary for commits 82e74db..3fdc42d. View details on DeepSource ↗.

Analysis Summary

Analyzer	Status	Summary	Link
C#	✅ Success		View Check ↗

---

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

[smellilac]

Git asking to add to name of "LogMessage" method, because because it sees Task in the method return parameter, but I use Task.CompletedTask; so it doesn't make sense

[HEJOK254]

Git asking to add to name of "LogMessage" method, because because it sees Task in the method return parameter, but I use Task.CompletedTask; so it doesn't make sense

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

[smellilac]

Git asking to add to name of "LogMessage" method, because because it sees Task in the method return parameter, but I use Task.CompletedTask; so it doesn't make sense

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. I think it would be more misleading than reflective of the asynchronicity.

[HEJOK254]

In this case it is not necessary, since the method does not perform asynchronous operations. I think it would be more misleading than reflective of the asynchronicity.

In this case, the issue can be ignored by adding a comment in the same line, or the line above: skipcq: CS-R1073

[smellilac]

In this case it is not necessary, since the method does not perform asynchronous operations. I think it would be more misleading than reflective of the asynchronicity.

In this case, the issue can be ignored by adding a comment in the same line, or the line above: skipcq: CS-R1073

It was good idea :)

[HEJOK254]

Sorry for the delay, I'm going to try to review the pr today and suggest some changes if needed

[smellilac]

You don't merge PR. Want to add/correct something?

[HEJOK254]

You don't merge PR. Want to add/correct something?

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

[smellilac]

delay

No problem. Glad to have you back!

[smellilac]

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.

[smellilac]

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?

[HEJOK254]

However, I can clean the code and upload the commit.

Yeah, you can do that if you want. I was basically thinking of fixing the indentations and formatting, plus maybe removing the unused using directives. All of that can be achieved by running a clean-up in Visual Studio, as far as I'm aware, and additionally double-checking if everything is correct and works properly.

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

[smellilac]

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.

[HEJOK254]

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

[smellilac]

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]

I forgot to remove that $ sign in SerilogConfiguration.cs in the review suggestion, which was causing the DeepSource issue, and I also forgot to add a suggestion to remove the Serilog Enricher, which is now unused, so I went ahead and committed the fixes myself, hope that's not an issue

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 :)

[smellilac]

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!

[HEJOK254]

No questions or suggestions.

Ok, I'll merge it now

Thanks for helping me with the project :)