Project-Books / book-project

Book tracker web app for book lovers
https://project-books.github.io/
GNU General Public License v3.0
482 stars 457 forks source link

Remove duplicated interface #918

Closed knjk04 closed 2 years ago

knjk04 commented 2 years ago

Is your feature request related to a problem? If so, please describe. Two ExcludeFromJacocoGeneratedReport.java files exist: in book-app and in the email-service.

Describe the solution you'd like Only have one occurrence of the file.

Additional context

If you need help with anything, we'll be happy to help you over a GitHub Q&A discussion or our Slack workspace

poshi1865 commented 2 years ago

Hi! I would like to work on this issue, can I be assigned to this? Also, may I know which occurance of that file should remain? The one in the email-service or the one in the book-app? Because I don't find any usages of ExcludeFromJacocoGeneratedReport.

knjk04 commented 2 years ago

Hi @poshi1865! Thanks for offering to help with this, I'll assign you to it!

We are using the ExcludeFromJacocoGeneratedReport annotation (e.g. here: https://github.com/Project-Books/book-project/blob/main/backend/book-app/src/main/java/com/karankumar/bookproject/account/auth/listener/AuthenticationBadCredentialsListener.java#L29). Perhaps try searching for ExcludeFromJacocoGeneratedReport rather than any usages to see if that brings up any search results.

We want the ExcludeFromJacocoGeneratedReport.java annotation to be used in both the bookapp and the email-service.

jaymeriegel commented 2 years ago

Hi @knjk04 and @poshi1865,

I really enjoyed the project and will try help with futures issues. In this one in particular, the best would not be to create a module "commons" to put classes, enums, interfaces, etc... common to the project?

thanks

knjk04 commented 2 years ago

Hi @jaymeriegel, that's a good idea!

poshi1865 commented 2 years ago

Hi @jaymeriegel, thanks for the suggestion! Also @knjk04 my approach that I am planning to implement is to declare a dependency to the book-app module in email-service and then we can use the interface from book-app in email-service through a normal import. Another approach can be to create a separate module called commons as @jaymeriegel pointed out. What do you think?

knjk04 commented 2 years ago

Hi @poshi1865, let's go with the commons module. Thanks!

poshi1865 commented 2 years ago

@knjk04 I have made a pull request, please review and let me know if everything looks alright.