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

Add a step option for increment integer provider #30

Merged

Conversation

bhagenbourger
Copy link
Contributor

#19

@bhagenbourger bhagenbourger marked this pull request as draft March 5, 2024 12:56
@bhagenbourger bhagenbourger force-pushed the feature/increment_by_step branch 3 times, most recently from 97035cc to 7e3c5e7 Compare March 8, 2024 08:26
@bhagenbourger bhagenbourger marked this pull request as ready for review March 8, 2024 16:47
Copy link
Collaborator

@vianneybacoup vianneybacoup 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 PR, exactly what I had in mind !

A small comment tho, to keep the possibility of step == 0 as it is giving an expected output

let step_option = column["step"].as_i64().unwrap_or(DEFAULT_STEP) as i32;
let mut param_step: i32 = DEFAULT_STEP as i32;

if step_option != 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It does not make sense to have a step of 0 you are right, but the behavior is not bad and has an expected logic
So we think that being able to put 0 can be kept

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback @vianneybacoup
I amended my commit to allow 0 value for step option.

@bhagenbourger bhagenbourger force-pushed the feature/increment_by_step branch from 7e3c5e7 to cfc0dfe Compare March 9, 2024 13:07
@vianneybacoup vianneybacoup merged commit 827e4e1 into soma-smart:main Mar 10, 2024
3 checks passed
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.

2 participants