Skip to content

Conversation

andsel
Copy link
Contributor

@andsel andsel commented Apr 1, 2025

Release notes

[rn:skip]

What does this PR do?

Convert the Ruby settings classes Integer and PositiveInteger to Java, renaming them to SettingInteger and SettingPositiveInteger.

Why is it important/What is the impact to the user?

As a developer I want all the settings hierarchy to be implemented in a statically typed language, Java.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

@andsel andsel self-assigned this Apr 1, 2025
@mergify
Copy link
Contributor

mergify bot commented Apr 1, 2025

This pull request does not have a backport label. Could you fix it @andsel? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.
  • backport-8.x is the label to automatically backport to the 8.x branch.
  • If no backport is necessary, please add the backport-skip label

@elastic-sonarqube
Copy link

@andsel andsel changed the title Feature/integers setting to java Convert Ruby Integer and PositiveInteger settings classes to Java Apr 1, 2025
@andsel andsel force-pushed the feature/integers_setting_to_java branch from 500ee82 to 0e99a1a Compare June 24, 2025 12:45
@elastic-sonarqube
Copy link

@andsel andsel force-pushed the feature/integers_setting_to_java branch from 0e99a1a to 7edc640 Compare August 8, 2025 14:02
@andsel andsel marked this pull request as ready for review August 8, 2025 16:30
Copy link
Member

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

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

This looks correct, would it be possible to add some tests to cover our coerce logic for doing String -> PositiveInt?

@elastic-sonarqube
Copy link

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @andsel

@andsel andsel requested a review from donoghuc August 11, 2025 08:44
Copy link
Member

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

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

Thanks for the additional tests!

@andsel andsel merged commit aabf84b into elastic:main Aug 12, 2025
12 checks passed
donoghuc added a commit to donoghuc/logstash that referenced this pull request Aug 15, 2025
Recently elastic#17460 the implementation for
integer/postive integer settings was moved from jruby to pure java. The ruby
implementation was leinient with whitespace in string values. This commit
updates the java implementation to replicate this by stripping whitespace from
string values before trying to parse them as int.
donoghuc added a commit that referenced this pull request Aug 15, 2025
Recently #17460 the implementation for
integer/postive integer settings was moved from jruby to pure java. The ruby
implementation was leinient with whitespace in string values. This commit
updates the java implementation to replicate this by stripping whitespace from
string values before trying to parse them as int.
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.

Reimplement LogStash::Setting::Integer and LogStash::Setting::PositiveInteger to Java

3 participants