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

#51: Simplify usage by offering combined classes #52

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

benedeki
Copy link
Contributor

@benedeki benedeki commented Jul 6, 2023

  • DBMultipleResultFunction added
  • DBMultipleResultFunctionWithStatusSupport added
  • DBOptionalResultFunction added
  • DBOptionalResultFunctionWithStatusSupport added
  • DBSingleResultFunction added
  • DBSingleResultFunctionWithStatusSupport

Closes #51

* `DBMultipleResultFunction` added
* `DBMultipleResultFunctionWithStatusSupport` added
* `DBOptionalResultFunction` added
* `DBOptionalResultFunctionWithStatusSupport` added
* `DBSingleResultFunction` added
* `DBSingleResultFunctionWithStatusSupport`
@benedeki benedeki self-assigned this Jul 6, 2023
@benedeki benedeki requested a review from lsulak July 6, 2023 13:13
Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM although I think that there is a big potential for a lot of code deletion. Please see my comments

  • code reviewed
  • pulled
  • built
  • ran tests
  • ran against a DB

Co-authored-by: Ladislav Sulak <laco.sulak@gmail.com>
}

/**
@see [[za.co.absa.fadb.DBFunction.DBSingleResultFunction]]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw I would be as harsh as this - let's remove all the code doc (these params) and have a "one liner" containing just this @see for the whole class

@benedeki benedeki added the work in progress Work on this item is not yet finished (mainly intended for PRs) label Jul 11, 2023
@benedeki
Copy link
Contributor Author

Doc is suddenly failing - on a class that has not been changed. But found other (naming) issues, that I want to address.

@salamonpavel
Copy link
Contributor

The topic of this PR has been addressed in PR #106. I believe we can close this one.


object DBFunction {
/**
* Combines [[za.co.absa.fadb.DBFunction.DBMultipleResultFunction DBMultipleResultFunction]] with Slick and Postgres support
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Combines [[za.co.absa.fadb.DBFunction.DBMultipleResultFunction DBMultipleResultFunction]] with Slick and Postgres support
* Combines [[za.co.absa.fadb.DBFunction.DBMultipleResultFunction]] with Slick and Postgres support

}

/**
* Combines [[za.co.absa.fadb.DBFunction.DBSingleResultFunction DBSingleResultFunction]] with Slick and Postgres support
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Combines [[za.co.absa.fadb.DBFunction.DBSingleResultFunction DBSingleResultFunction]] with Slick and Postgres support
* Combines [[za.co.absa.fadb.DBFunction.DBSingleResultFunction]] with Slick and Postgres support

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and so on for the rest of them below

Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see much value in this PR to be fair :( I think that users should combine whatever they want and that this code brings only very little convenience, but the drawback is that it's a code and each code must be maintained, tested, understood, and documented.

Besides, I think that something like this was already implemented recently and thus I propose to close this PR. For Slick, see this: https://github.com/AbsaOSS/fa-db/pull/106/files#diff-b585ca0e03975d3e0dbdbedca5e6a38f067b043218992ebf9e20dc03b98af2c4 and Doobie has its equivalent in relevant files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress Work on this item is not yet finished (mainly intended for PRs)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify usage by offering combined classes
3 participants