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

#99: DBEngine requirement removed from DBSchema. Fixes #99. #100

Closed
wants to merge 2 commits into from

Conversation

salamonpavel
Copy link
Contributor

DBEngine requirement was removed from the DBSchema class.

Copy link

github-actions bot commented Nov 30, 2023

JaCoCo code coverage report - scala 2.11.12

File Coverage [94.83%] 🍏
DBSchema.scala 94.83% 🍏
Total Project Coverage 49.75% 🍏

Copy link

github-actions bot commented Nov 30, 2023

JaCoCo code coverage report - scala 2.12.17

File Coverage [93.88%] 🍏
DBSchema.scala 93.88% 🍏
Total Project Coverage 50.04% 🍏

Copy link
Contributor

@miroslavpojer miroslavpojer left a comment

Choose a reason for hiding this comment

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

  • pulled
  • built
  • code review
  • code coverage - proposed 2 tests by comment.

@@ -18,27 +18,17 @@ package za.co.absa.fadb
import org.scalatest.funsuite.AnyFunSuite
import za.co.absa.fadb.naming.implementations.SnakeCaseNaming.Implicits.namingConvention

import scala.concurrent.{ExecutionContext, Future}

class DBSchemaSuite extends AnyFunSuite {
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to add new tests to improve coverage:

test("schema name with naming convention without override") {
    object LowerCaseNamingConvention extends NamingConvention {
      def stringPerConvention(original: String): String = original.toLowerCase
    }
    class Bar extends DBSchema(LowerCaseNamingConvention, null)

    val schema = new Bar
    assert(schema.schemaName == "bar") // Assuming the naming convention converts "Bar" to "bar"
  }

test("schema name with naming convention with override") {
    object LowerCaseNamingConvention extends NamingConvention {
      def stringPerConvention(original: String): String = original.toLowerCase
    }
    class Bar extends DBSchema(LowerCaseNamingConvention, "bar")

    val schema = new Bar
    assert(schema.schemaName == "bar")
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @miroslavpojer. Will add it to the codebase. I think also that once we merge this code we should revisit existing projects utilizing this library, fix it's usage there and then update examples so users of the library have updated examples as part of this repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

object LowerCaseNamingConvention extends NamingConvention {
def stringPerConvention(original: String): String = original.toLowerCase
}
class Bar extends DBSchema(LowerCaseNamingConvention, "bar")
Copy link
Collaborator

@lsulak lsulak Dec 14, 2023

Choose a reason for hiding this comment

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

maybe I would chose something other than bar, just to distinguish it? :) I mean, so that the schema name is actually overridden

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.

Almost approved, found a tiny thing. Since originally there was a dependency on the DBEngine, but it wasn't really used here, I like the changes

@salamonpavel
Copy link
Contributor Author

This PR has become obsolete.

@lsulak lsulak deleted the bug/99-dependency-propagation-dbschema branch February 26, 2024 15:43
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.

3 participants