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

#23: normalizing Scala -> PG objects #113

Merged
merged 7 commits into from
Nov 8, 2023

Conversation

lsulak
Copy link
Collaborator

@lsulak lsulak commented Nov 7, 2023

Related to #23, but won't fully implement everything in that ticket. I'll split it into more PRs because #23 is dependant on DB code that is still not fully done&merged.

Copy link

github-actions bot commented Nov 7, 2023

JaCoCo agent module code coverage report - spark:2 - scala 2.12.12

There is no coverage information present for the Files changed

Total Project Coverage 83.26% 🍏

Copy link

github-actions bot commented Nov 7, 2023

JaCoCo server module code coverage report - scala 2.12.12

File Coverage [19.14%]
PrototypeController.scala 54.55%
Runs.scala 17.17%
Total Project Coverage 17.17%

}

private def scalaSeqToPgArray(toConvert: Seq[String]): String = {
val scalaArrayAndItsContent: Regex = "^\\[(.*)\\]$".r
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment what the Regex matches.
Also I have a suspicion, it might not be correct. 😟

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refactored


private def scalaSeqToPgArray(toConvert: Seq[String]): String = {
val scalaArrayAndItsContent: Regex = "^\\[(.*)\\]$".r
val scalaSeqJsonized = SerializationUtils.asJson(toConvert)
Copy link
Contributor

Choose a reason for hiding this comment

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

"Jsonizing" any string? Do we really want/need that? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Explained via a quick chat

benedeki
benedeki previously approved these changes Nov 8, 2023
Copy link
Contributor

@benedeki benedeki left a comment

Choose a reason for hiding this comment

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

Maybe there's a generic recursive solution, but this is good and enough:

  • code reviewed
  • pulled
  • built
  • run

Comment on lines 46 to 47
val scalaSeqJsonized = SerializationUtils.asJson(toConvert) // this also correctly escapes double quotes
"{" + scalaSeqJsonized.substring(1, scalaSeqJsonized.length - 1) + "}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor:

Suggested change
val scalaSeqJsonized = SerializationUtils.asJson(toConvert) // this also correctly escapes double quotes
"{" + scalaSeqJsonized.substring(1, scalaSeqJsonized.length - 1) + "}"
val scalaSeqJsonized = SerializationUtils.asJson(toConvert) // this also correctly escapes double quotes
"{" + scalaSeqJsonized.substring(1, scalaSeqJsonized.length - 1) + "}" //...only the square brackets have to be replaced with curly ones

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's better, thanks, I'll change it :)

@lsulak lsulak merged commit 8ad1b36 into master Nov 8, 2023
4 of 5 checks passed
@lsulak lsulak deleted the feature/23-fa-db-entities-into-scala-code branch November 8, 2023 14:05
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.

2 participants