Skip to content

Conversation

@benedeki
Copy link
Contributor

@benedeki benedeki commented Feb 4, 2022

  • Separated the core and the Slick implementation
  • Core is now independent and type-parametrized
  • Enceladus moved to examples folder
  • Deleted all kind unneeded code

@benedeki
Copy link
Contributor Author

benedeki commented Feb 4, 2022

This might be easier to review as the whole code rather than a PR with code compare. /methinks

benedeki and others added 2 commits February 4, 2022 13:18
Co-authored-by: Saša Zejnilović <zejnils@gmail.com>
@benedeki benedeki mentioned this pull request Feb 4, 2022
Copy link

@dk1844 dk1844 left a comment

Choose a reason for hiding this comment

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

LGTM (just read the code). To the degree that I have understood it, I see nothing wrong.

# Conflicts:
#	src/main/scala/za/co/absa/faDB/DBFunction.scala
*/

package za.co.absa.faDB.namingConventions.lettersCase
package za.co.absa.fadb.naming_conventions.lettersCase

Choose a reason for hiding this comment

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

Probably lettersCase package should also be renamed

dk1844
dk1844 previously approved these changes Feb 10, 2022
Copy link

@dk1844 dk1844 left a comment

Choose a reason for hiding this comment

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

LGTM (just read the code)

pull_request:
branches: [ master, develop ]

jobs:
Copy link

Choose a reason for hiding this comment

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

The code of the license check seems ok, but I would expect it to have a direct effect - for the PR to actually launch the check -- which does not appear to be the case.

Could it be perhaps that the correct path should be lowercased? (.github/workflows/) as described in the doc?

Comment on lines 4 to 8
This library is a less traditional way to facilitate data between an application and an SQL Database.

Traditionally application directly applies SQL queries or use some ORM framework. While the first approach mixes two
rather different domain languages within one source, the second too often fails in case of more complicated queries and
table relations.
Copy link

Choose a reason for hiding this comment

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

Suggested change
This library is a less traditional way to facilitate data between an application and an SQL Database.
Traditionally application directly applies SQL queries or use some ORM framework. While the first approach mixes two
rather different domain languages within one source, the second too often fails in case of more complicated queries and
table relations.
This library is a less traditional way to facilitate data between an application and an SQL Database.
Traditionally, an application directly applies SQL queries or uses an ORM framework. While the first approach mixes two
rather different domain languages within one source, the second too often fails in case of more complicated queries and
table relations.

import org.scalatest.matchers.should.Matchers
import org.scalatest.wordspec.AnyWordSpec
import za.co.absa.faDB.namingConventions.lettersCase.LettersCase._
import za.co.absa.fadb.naming_conventions.letters_case.LettersCase._
Copy link

@dk1844 dk1844 Feb 10, 2022

Choose a reason for hiding this comment

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

Suggested change
import za.co.absa.fadb.naming_conventions.letters_case.LettersCase._
import za.co.absa.fadb.namingconventions.letterscases.LettersCase._

or

Suggested change
import za.co.absa.fadb.naming_conventions.letters_case.LettersCase._
import za.co.absa.fadb.naming.letterscases.LettersCase._

While I am am aware that both this.is.a.possible.package_name and this.is.a.possible.packagename.too are conforming the general java/scala naming convention, I personally favor the latter.

@benedeki benedeki changed the base branch from First_POC to master February 11, 2022 09:26
@benedeki benedeki dismissed dk1844’s stale review February 11, 2022 09:26

The base branch was changed.

Copy link

@AdrianOlosutean AdrianOlosutean left a comment

Choose a reason for hiding this comment

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

Looks good

@benedeki benedeki merged commit 5c6f641 into master Feb 11, 2022
@benedeki benedeki deleted the Separating_DB_engine_and_Core branch March 13, 2022 11:40
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.

4 participants