Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Doobie multiple results with aggregated status: 219 #220
Doobie multiple results with aggregated status: 219 #220
Changes from 31 commits
c582b79
da3f9f6
a85e672
2de71dd
0176678
6a3474f
1f6b20a
b1bbd32
166f506
953a99a
075c621
933688b
d70a8c1
c5dc61f
b31d9a5
ee594e4
d684420
1908297
fa6e076
d51b81d
bff113b
dce3de7
d53a58a
1a4bfb1
6b90db1
9292faa
e26ce12
bb2b202
f02bf72
c96e90a
3005404
94750ce
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not the best practice to define such implicits in package object as by doing that you implicitly import them into every possible scope in the entire package which might leaad to unintended implicit conversions or resolutions. These types should be changed. AdditionalDataDTO will undergo some changes when working on endpoints related to AdditionalData. PartitioningDTO could be modeled as case class and json serde defined in its companion or kept as is but the serde defined elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serviceCallWithStatus not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, will remove the method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these instances were placed in fa-db. Therefore, there is no need to keep them in Atum anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests for this class are missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better approach:
While not incorrect, better approach, and enuring better compatibility, is to use the
super.fieldsToSelect ++
instead of naming explicitly naming"status", "status_text",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Let's use implicits from fa-db. Btw since the doobie-postgres-circe has been made a dependency of doobie's module in fa-db there is no need to explicitly build the project with that dependency.
In other words the below dependency is not needed anymore as it's brought in transitively by the fa-db's doobie module. Pls remove it.
lazy val pgCirceDoobie = "org.tpolecat" %% "doobie-postgres-circe" % "1.0.0-RC2"