[OPIK-844] Add support for span comments #1137
Conversation
apps/opik-backend/src/main/java/com/comet/opik/domain/SpanDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/SpanDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/SpanDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/SpanDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/CommentService.java
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/SpanDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/AssertionUtils.java
Outdated
Show resolved
Hide resolved
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| @RequiredArgsConstructor | ||
| public abstract class BaseTestClient { |
There was a problem hiding this comment.
BaseCommentResourceClient
There was a problem hiding this comment.
I suggest to keep the current name. It's not an interface, but a base class, and not only comments could be added there. For example feedback-scores related endpoints could be moved there too. Plus we could move there some common code, like authentication related tests, which are now just copypasted for all Resource tests.
There was a problem hiding this comment.
It's too generic, and adding too much logic from different entities will make it difficult to maintain. It's better to separate responsibilities and favor composition if more entities are needed.
There was a problem hiding this comment.
This is test client, and only logic here is for calling endpoints. So functions count here would be not more than number of endpoints per class. And considering that we could put here only endpoints which are same between classes it will be even less. Do not see how it might become with too much logic.
There was a problem hiding this comment.
I'm approving as it is not critical, but unscoped classes tend to grow and acquire too much responsibility; if we don't scope, another engineer may see this as a utils class.
There was a problem hiding this comment.
I renamed it as you suggested. In future I guess we could add new BaseTestClient where we could put endpoints which are same for almost all classes, like create, getById, batchDelete...
But it's too much refactoring for this task:)
I never planned this to be an utility class, only a centralized client to call endpoints. I guess we should separate these concerns in tests.
There was a problem hiding this comment.
Great, we can start adding some technical debit tickets to the backlog so we don't forget it.
apps/opik-backend/src/main/java/com/comet/opik/domain/CommentService.java
Outdated
Show resolved
Hide resolved
BorisTkachenko
force-pushed
the
boryst/OPIK-844-add-support-for-span-comments
branch
from
dde95f8 to
f7b84eb
Compare
BorisTkachenko
deleted the
boryst/OPIK-844-add-support-for-span-comments
branch
Details
Add support for span comments
Testing
Integration tests