-
Notifications
You must be signed in to change notification settings - Fork 263
Add NotionDatabaseItemReaderBuilder #202
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
base: main
Are you sure you want to change the base?
Add NotionDatabaseItemReaderBuilder #202
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just a few style comments.
| .build(); | ||
|
|
||
| // THEN | ||
| Assertions.assertThat(ReflectionTestUtils.getField(reader, "token")).isEqualTo(expectedToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?