Add NotionDatabaseItemReaderBuilder#202
Add NotionDatabaseItemReaderBuilder#202qlfyd123 wants to merge 7 commits intospring-projects:mainfrom
Conversation
Signed-off-by: qlfyd123 <hajewoong@gmail.com>
90adb95 to
1693114
Compare
|
@qlfyd123 thanks for raising the PR!
Yes, it was intentional, as I'd prefer to avoid
Sure, it sounds good to me 👍
Keeping the |
scordio
left a comment
There was a problem hiding this comment.
Thanks, just a few style comments.
| .build(); | ||
|
|
||
| // THEN | ||
| Assertions.assertThat(ReflectionTestUtils.getField(reader, "token")).isEqualTo(expectedToken); |
There was a problem hiding this comment.
I'd favor then from BDDAssertions (statically imported) and extracting:
| Assertions.assertThat(ReflectionTestUtils.getField(reader, "token")).isEqualTo(expectedToken); | |
| then(reader).extracting("token").isEqualTo(expectedToken); |
| // WHEN & THEN | ||
| Assertions.assertThatThrownBy(builder::build).isInstanceOf(NullPointerException.class); |
There was a problem hiding this comment.
I'd favor catchException statically imported from BDDAssertions:
| // WHEN & THEN | |
| Assertions.assertThatThrownBy(builder::build).isInstanceOf(NullPointerException.class); | |
| // WHEN | |
| Exception exception = catchException(builder::build); | |
| // THEN | |
| then(exception).isInstanceOf(NullPointerException.class); |
| @Test | ||
| void should_succeed() { | ||
| // GIVEN | ||
| String expectedToken = "FOO TOKEN"; |
There was a problem hiding this comment.
Given that there are no other clashing variables, I'd remove the "expected" prefix from all these:
| String expectedToken = "FOO TOKEN"; | |
| String token = "FOO TOKEN"; |
Signed-off-by: qlfyd123 <hajewoong@gmail.com>
Signed-off-by: qlfyd123 <hajewoong@gmail.com>
Signed-off-by: qlfyd123 <hajewoong@gmail.com>
|
thx for review @scordio I have addressed all the feedback from the review. Specifically, I have updated the null checks to use |
|
Thanks! Would you like to update the README, too?
I can also take care of it. |
Signed-off-by: qlfyd123 <hajewoong@gmail.com>
Signed-off-by: qlfyd123 <hajewoong@gmail.com>
Signed-off-by: qlfyd123 <hajewoong@gmail.com>
|
I updated README.md file and fix javadoc for ci workflow |
Changes
Created a builder class for
NotionDatabaseItemReaderand added corresponding test code for the builder.This pr is related with #151
Questions/Suggestions
1. Inconsistency between documentation and implementation for
sortsdefault valueAccording to the GitHub README, the default value for
sortsisnull. However, in the actualNotionDatabaseItemReaderclass, it's initialized with an emptySortarray:Could you clarify whether this is intentional or if it needs to be aligned with the documentation?
2. Suggestion to use
Assert.notNullinstead ofObjects.requireNonNullin constructorCurrently, the constructor uses
Objects.requireNonNullfor parameter validation:I suggest using
Assert.notNullinstead to provide clearer, more descriptive error messages, which would be more consistent with Spring conventions.3. Missing
@Nullableannotations on setter parameters causing IDE warningsGiven the current project configuration with
@NullMarkedat the package level:All method parameters are treated as non-null by default. However, some setter methods in the reader class accept nullable parameters (e.g.,
setFilter) but lack@Nullableannotations. This causes IDE warnings when using the builder.Would it be acceptable to add
@Nullableannotations to the appropriate setter parameters to align with the null-safety contract?