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

Add custom log formatter, remove slf4j dependency #741

Merged
merged 3 commits into from Oct 28, 2023
Merged

Conversation

sedadas
Copy link
Contributor

@sedadas sedadas commented Oct 28, 2023

Second part of #740

@afdia afdia marked this pull request as ready for review October 28, 2023 14:00
@afdia
Copy link
Collaborator

afdia commented Oct 28, 2023

Looks good thanks!

One note for future commits: I'm not saying that it's bad, but to spare you some time which could be better invested in other stuff: You don't have to write javadoc for every method (especially since Umlet is not a library which others use without viewing the Umlet code itself nor will there be external implementations of our interfaces which would benefit from exact javadoc specs).

In general I would suggest to limit comments (or javadoc) to code which is not intuitively understood by a typical Java dev.

Or let's put it that way: put yourself in the shoes of a developer who doesn't know your new code and use comments to increase the understandability of your code. E.g. for background knowledge like "//approach x chosen, because y would have problem z" or for stuff which is not immediately obvious (to think about a concrete example in your commit: "//using Date instead of LocalDateTime because logger should work in GWT which only suports Date")

It will spare you some time and we will still have the most important additional information which is not obvious in the code. Of course you could also add comments to "old Umlet code" which you had problems to understand if you think it will help other developers :)

@afdia afdia merged commit a8e583a into umlet:master Oct 28, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants