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

Updated conditional on job bean to support kabob case. #795

Closed
wants to merge 3 commits into from

Conversation

cppwfs
Copy link
Collaborator

@cppwfs cppwfs commented Aug 12, 2021

Also added comments reader and writer properties so that boot's metadata resolver can utilize them.

resolves #794

@@ -78,7 +78,7 @@ private void validateProperties(SingleStepJobProperties properties) {

@Bean
@ConditionalOnMissingBean
@ConditionalOnProperty(prefix = "spring.batch.job", name = "jobName")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be sufficient to change this line to
@ConditionalOnProperty(prefix = "spring.batch.job", name = "job-name")
If name is given in kebab case, then it will also match camel case. But not the other way around.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hpoettker great catch! Thanks!

Copy link

@Buzzardo Buzzardo left a comment

Choose a reason for hiding this comment

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

We should change "if" to "whether" in all these cases because it is best to use "if" only to introduce conditional material and use "whether" to present two choices. It's both a matter of convention and a matter of clarity. Merriam Webster gives a good take on it: https://www.merriam-webster.com/words-at-play/if-vs-whether-difference-usage

private boolean enabled;

/**
* Establishes whether the {@link Jackson2JsonMessageConverter} is to be used as a
* message converter. Defaults to true.

Choose a reason for hiding this comment

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

Change the double space to a single space and make "true" be code: {@code true}

@@ -26,8 +26,15 @@
@ConfigurationProperties(prefix = "spring.batch.job.amqpitemwriter")
public class AmqpItemWriterProperties {

/**
* Enables or disables the AmqpItemWriter. Defaults to false.

Choose a reason for hiding this comment

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

Change the double space to a single space and make "false" be code: {@code false}

private boolean enabled;

/**
* Establishes whether the {@link Jackson2JsonMessageConverter} is to be used as a
* message converter. Defaults to true.

Choose a reason for hiding this comment

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

Make the double space be a single space and make "true" be code: {@code true}

@@ -39,7 +46,7 @@ public boolean isEnabled() {
}

/**
* Enables or disables the AmqpItemReader.
* Enables or disables the AmqpItemWriter.

Choose a reason for hiding this comment

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

Make "AmqpItemWriter" be code: {@code AmqpItemWriter}

@@ -39,7 +46,7 @@ public boolean isEnabled() {
}

/**
* Enables or disables the AmqpItemReader.
* Enables or disables the AmqpItemWriter.
* @param enabled if true then AmqpItemWriter will be enabled. Defaults to false.

Choose a reason for hiding this comment

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

Make "true" be code: {@code true}
Make "AmqpItemWriter" be code: {@code AmqpItemWriter}
Make "false" be code: {@code false}
Change "will be" to "is".

Glenn Renfro added 2 commits September 17, 2021 14:08
Also added comments  reader and writer properties so that boot's metadata resolver can utilize them.

resolves spring-cloud#794
@cppwfs
Copy link
Collaborator Author

cppwfs commented Sep 27, 2021

@mminella approved PR.
Squashed, merged, rebased, backported.

@cppwfs cppwfs closed this Sep 27, 2021
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.

Single Step Batch Jobs needs to support different mutations of jobName property
4 participants