Skip to content

Added @Nullable annotation to overriding methods. #726

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

Closed
wants to merge 1 commit into from

Conversation

mikeldpl
Copy link
Contributor

@mikeldpl mikeldpl commented Sep 1, 2019

Resolves BATCH-2839

@@ -102,6 +103,7 @@ protected void doExecute(StepExecution stepExecution) throws Exception {
/* (non-Javadoc)
* @see org.springframework.batch.core.step.StepLocator#getStep(java.lang.String)
*/
@Nullable
Copy link
Contributor

Choose a reason for hiding this comment

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

This @Nullable here does not make sense as the implementation throws NoSuchStepException when no step is found with the given name. The issue is that the behaviour is not consistent across the framework: some implementations return null (like SimpleJob) and some others throw NoSuchStepException like in here. I believe it is the contract StepLocator#getStep that should be fixed to throw an exception instead of returning null (in order to make it consistent with JobLocator), but my concern is that it would be a breaking change.

Copy link
Contributor

@fmbenhassine fmbenhassine Sep 17, 2019

Choose a reason for hiding this comment

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

It looks like the contract StepLocator has been changed incorrectly here. I reverted that change and adapted the PR accordingly (See 2306141).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. Thanks for the changes.

@@ -189,7 +191,8 @@ protected T doRead() throws Exception {
/**
* @return next line (skip comments).getCurrentResource
*/
private String readLine() {
@Nullable
protected String readLine() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The visibility of readLine has been changed from private to protected. Why is that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in 2306141 as I believe this should not change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done for my internal tests. Thanks for the change.

@fmbenhassine
Copy link
Contributor

@mikeldpl Could you please update the year in the license headers of the changed files? Thank you upfront.

@fmbenhassine fmbenhassine self-assigned this Sep 16, 2019
@fmbenhassine
Copy link
Contributor

Rebased and merged as a7092a2.

@mikeldpl Thank you for your PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants