Conversation
WalkthroughA new reflection-based utility Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Extractor as ClassFieldNamesExtractor
participant Reflect as ScalaReflect
participant Naming as NamingConvention
Client->>Extractor: extract[T]() / extract[T](namingConvention)
Extractor->>Reflect: inspect TypeTag[T] → type info & primary constructor
alt T is class or case-class
Reflect-->>Extractor: constructor param lists
Extractor->>Extractor: flatten param lists, collect names
Extractor->>Naming: map names via namingConvention
Naming-->>Extractor: transformed names
Extractor-->>Client: Seq[String]
else T is primitive/trait/non-class
Reflect-->>Extractor: type info indicates invalid target
Extractor-->>Client: throws IllegalArgumentException
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…constructor-parameter-names
|
missing licence header and consider to rewrite the IF logic to make the code less nested (IF NOT THEN EXCEPTION kind of a thing) |
…constructor-parameter-names
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/src/main/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractor.scala (1)
6-6: Remove unused import.The
java.langimport does not appear to be used anywhere in this file.-import java.lang import scala.reflect.runtime.universe._core/src/test/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractorUnitTests.scala (1)
53-57: Add parentheses for consistency.Line 55 is missing parentheses when calling
extract[TestTrait], which is inconsistent with the other test at line 61 that usesextract[Boolean]. While both are valid Scala syntax, using parentheses consistently improves readability.test("Extract from trait fails") { intercept[IllegalArgumentException] { - ClassFieldNamesExtractor.extract[TestTrait] + ClassFieldNamesExtractor.extract[TestTrait]() } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractor.scala(1 hunks)core/src/test/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractorUnitTests.scala(1 hunks)project/Dependencies.scala(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: License Check
core/src/main/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractor.scala
[error] 1-1: There are files without headers! (headerCheck) faDbCore / Compile / headerCheck
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build, Test and Doc (2.12.17)
- GitHub Check: Build, Test and Measure
🔇 Additional comments (8)
core/src/main/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractor.scala (2)
43-47: LGTM: Implicit naming convention handling.The method correctly provides a default
SnakeCaseNamingconvention while allowing it to be overridden via implicit scope.
49-51: LGTM: Explicit naming convention overload.The explicit overload provides a clean way to pass a naming convention without relying on implicits.
core/src/test/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractorUnitTests.scala (5)
25-32: LGTM: Default naming convention test.The test correctly validates that case class fields are extracted and transformed using the default
SnakeCaseNamingconvention.
34-41: LGTM: Explicit naming convention test.The test properly validates explicit naming convention usage with
AsIsNaming(UpperCase).
43-51: LGTM: Implicit naming convention test.The test correctly validates that an implicit naming convention in scope is properly used by the extractor.
59-63: LGTM: Primitive type validation test.The test correctly validates that primitive types are rejected with an appropriate exception.
67-82: LGTM: Well-structured test fixtures.The test fixtures cover the necessary scenarios: case class, regular class, and trait. They are appropriately scoped within the companion object.
project/Dependencies.scala (1)
22-22: Scala-reflect dependency is compatible and secure.The project uses Scala 2.12.17 (primary) and 2.13.12 (cross-compiled), and the scala-reflect dependency at line 22 correctly uses
% scalaVersionto auto-match the project's Scala versions. The CVE-2022-36944 deserialization vulnerability affecting scala-library <2.13.9 is not present in Scala 2.13.12, and no direct vulnerabilities are currently reported for the scala artifacts. The dependency is compatible and introduces no known security risks.
* extended `.gitignore` for VS Code files
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractor.scala (1)
27-46: Consider handling overloaded constructors or documenting the single-constructor assumptionThe flattened checks for
!isClass,isPrimitive, andisTraitlook good and are much clearer now. One potential edge case is the constructor introspection:tpe .decl(termNames.CONSTRUCTOR) .asMethod .paramListsThis effectively assumes
Thas a single, non-overloaded constructor. For classes with multiple constructors, theCONSTRUCTORsymbol can be overloaded, and calling.asMethodmay fail or behave unexpectedly.If you expect
ClassFieldNamesExtractorto work only with case classes / classes that have a single primary constructor (which is likely in Fa‑db usage), it would help to either:
- Enforce that explicitly (e.g. inspect
.asTerm.alternativesand throw a clearIllegalArgumentExceptionwhen there are multiple constructors), or- Select the intended “primary” constructor explicitly and base
paramListson that.This would make the failure mode for multi-ctor classes deterministic and easier to debug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractor.scala(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build, Test and Doc (2.12.17)
🔇 Additional comments (2)
core/src/main/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractor.scala (2)
1-16: License header now present and consistent with project styleThe Apache 2.0 header is in place and formatted correctly, so the previous headerCheck/pipeline issue for this file should now be resolved.
54-62: Publicextractoverloads cleanly expose implicit and explicit naming conventionsThe two
extractoverloads—one using an implicitNamingConventionwith a sensible default and one taking it explicitly—provide a clear API surface and align with the goal of reusing existing naming conventions for field extraction. Delegation throughdoExtractkeeps behavior centralized.
JaCoCo
|
| Overall Project | 59.84% -1.22% |
🍏 |
|---|---|---|
| Files changed | 86% | 🍏 |
| File | Coverage | |
|---|---|---|
| ClassFieldNamesExtractor.scala | 84.78% -15.22% |
🍏 |
JaCoCo
|
| Overall Project | 70.17% | 🍏 |
|---|
There is no coverage information present for the Files changed
JaCoCo
|
| Overall Project | 91.42% | 🍏 |
|---|
There is no coverage information present for the Files changed
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/src/main/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractor.scala (2)
22-22: Remove unused import.The
java.langimport is not used anywhere in this file.Apply this diff:
import za.co.absa.db.fadb.naming.NamingConvention import za.co.absa.db.fadb.naming.implementations.SnakeCaseNaming -import java.lang import scala.reflect.runtime.universe._
60-62: Consider adding documentation for consistency.While the first
extractmethod has comprehensive Scaladoc, this overload lacks documentation. Consider adding a brief doc comment explaining when to use this explicit version versus the implicit version.Example documentation:
+ /** + * Extracts constructor field names from case class or regular class, and converts them according to naming convention. + * This is the explicit version that bypasses implicit resolution. + * @param namingConvention - the naming convention to use when converting the constructor parameters names into field name + * @tparam T - type to investigate and extract field names from + * @return - list of field names + */ def extract[T: TypeTag](namingConvention: NamingConvention): Seq[String] = {core/src/test/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractorUnitTests.scala (1)
24-63: Test coverage is solid.The test suite comprehensively covers:
- Default naming convention (snake_case)
- Explicit naming with uppercase
- Implicit naming with lowercase
- Error cases for traits and primitives
The expected values match the naming convention transformations correctly.
For completeness, consider adding edge case tests in a future iteration:
- Classes with empty constructors
- Classes with curried parameter lists (multiple parameter lists)
- Abstract classes
However, current coverage is sufficient for the initial implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractor.scala(1 hunks)core/src/test/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractorUnitTests.scala(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/test/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractorUnitTests.scala (2)
core/src/main/scala/za/co/absa/db/fadb/naming/LettersCase.scala (3)
LettersCase(33-55)UpperCase(52-54)LowerCase(45-47)core/src/main/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractor.scala (3)
ClassFieldNamesExtractor(25-64)extract(54-58)extract(60-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build, Test and Measure
- GitHub Check: Build, Test and Doc (2.13.12)
- GitHub Check: Build, Test and Doc (2.12.17)
🔇 Additional comments (3)
core/src/main/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractor.scala (1)
27-46: LGTM!The guard clause refactoring from previous reviews has been successfully applied. The logic is now clear, flat, and easy to follow. The validation checks are comprehensive and the extraction logic correctly extracts constructor parameters and applies the naming convention.
core/src/test/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractorUnitTests.scala (2)
1-22: LGTM!License header and imports are correct and appropriate for the test suite.
67-82: LGTM!The test fixtures are well-designed:
TestCaseClasstests case class extraction with additional methods to ensure only constructor params are extractedTestClasstests regular class extraction with additional vals and methods to verify proper filteringTestTraitprovides the negative test case for trait rejectionThe fixtures appropriately validate that only constructor parameters are extracted, not other class members.
| @@ -0,0 +1,64 @@ | |||
| /* | |||
| * Copyright 2022 ABSA Group Limited | |||
There was a problem hiding this comment.
Copyright year should be 2025.
The copyright year is 2022, but this is a new file being added in 2025. The test file ClassFieldNamesExtractorUnitTests.scala correctly uses 2025. Update the year to match.
Apply this diff:
-/*
- * Copyright 2022 ABSA Group Limited
+/*
+ * Copyright 2025 ABSA Group Limited📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Copyright 2022 ABSA Group Limited | |
| /* | |
| * Copyright 2025 ABSA Group Limited |
🤖 Prompt for AI Agents
In core/src/main/scala/za/co/absa/db/fadb/utils/ClassFieldNamesExtractor.scala
around line 2, the copyright header shows "2022" but should be "2025"; update
the year to 2025 to match the test file and repository conventions by replacing
2022 with 2025 in the file header.
| if (cl.isTrait) { | ||
| throw new IllegalArgumentException(s"${tpe.typeSymbol} is a trait, extraction is not supported") | ||
| } | ||
| tpe |
There was a problem hiding this comment.
I like the changes! I learned this technique when I read Clean Code book for the first time a while ago.
However, it's not equivalent, now this one is missing: if (cl.isCaseClass || cl.isClass) - probably only CaseClass, i.e. the first condition, is what we still need here? The second is probably addressed on few lines above, on the first condition actually (line 20)?
There was a problem hiding this comment.
The AI suugested the removal of the condition, claiming it's checking the same thing as above. And when I looked at the code, indeed, there's a constant true returned for isClass method. So indeed pointless.
Regarding the technique - IMHO it's slightly against the functional programming principles, if followed dogmatically. The return of the value, is from the point of the function, a side effect, and should happen "at the end of the world" only. Exiting the function prematurely, purist could claim, is against the principle.
But here when firing exceptions I think it's acceptable. 😉
There was a problem hiding this comment.
Hmm I would need to think hard about 'at the end of the world only' style, but I think that the code readability might be worse if followed blindly (nesting for example) in some circumstances (but on the other hand I symphatize with the benefit of having it in some scenarios).
However, throwing an exception is not the same as returning the value and existing 'successfully'. So IDK if it applies on those as well. Sometimes throughout the code flow you validate something and raise an exception and it would be less readable if you postpone with handling / raising at the end. IDK, but thanks for bringing it up
There was a problem hiding this comment.
On the implementation, I think I agree on isClass, but then on def isCaseClass: Boolean I am not sure - this actually has an implementation and I think is based on Reflection, so might still be relevant
There was a problem hiding this comment.
Yeah, but the condition checked if is a not a class and neither a case class, then it would fire the exception. As it's always a class the condition to throw the exception can never be met anymore, regardless of case class status.
There was a problem hiding this comment.
that's actually right. Ok, approving!
Closes #142
Summary by CodeRabbit
New Features
Tests
Chores
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.