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

Adds optional step param to expect_column_values_to_be_{in,de}creasing #316

Conversation

vitorbaptista
Copy link
Contributor

If set, the test will check that the values are not only strictly increasing/decreasing, but the difference between subsequent values is exactly step.

Issue this PR Addresses/Closes

Closes #315

Summary of Changes

Adds optional field step to tests expect_column_values_to_be_{in,de}creasing. If this value is set, it requires that the difference between subsequent values is exactly step. If it's not set, the behaviour of the test is the same as now (i.e., this isn't a breaking change).

The caveat is that it will only work for numeric columns. The test is literally value_field - value_field_lag = step.

I wrote this test for my own project. As I've written it already, I decided to sent this PR even without discussions. I'm able to do any changes required to get it merged. It's also OK if you don't think this is a good improvement to the project.

Why Do We Need These Changes

I have a column that must be strictly increasing, but the values need to have a difference of exactly 1 between them. So values like 1, 2, 3 are valid, but 1, 3, 4 are not.

Reviewers

@clausherther

@clausherther
Copy link
Contributor

@vitorbaptista vitorbaptista force-pushed the adds-step-to-increasing-and-decreasing-tests branch from cb6c382 to 534b6ff Compare August 16, 2024 21:09
@vitorbaptista
Copy link
Contributor Author

@clausherther Sure! It's done.

@clausherther
Copy link
Contributor

@vitorbaptista CI is failing b/c the macros also need to have step in the signature, with a default of None, e.g.

{% test expect_column_values_to_be_increasing(model, column_name,
                                                   sort_column=None,
                                                   strictly=True,
                                                   row_condition=None,
                                                   group_by=None,
                                                   step=None
) %}

If set, the test will check that the values are not only strictly
increasing/decreasing, but the difference between subsequent values is
exactly `step`.
@vitorbaptista vitorbaptista force-pushed the adds-step-to-increasing-and-decreasing-tests branch from 534b6ff to 636d67c Compare August 21, 2024 13:27
@vitorbaptista
Copy link
Contributor Author

Ops! I wrote that change locally but didn't commit it. Pushing the fix now.

@vitorbaptista
Copy link
Contributor Author

@clausherther just letting you know that AFAIK this is good to go ✌️

@clausherther
Copy link
Contributor

@vitorbaptista thanks for the reminder, merging!

@clausherther clausherther merged commit c9abed7 into calogica:main Sep 10, 2024
5 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.

[Feature Request] Allow defining the step between values on increasing/decreasing tests
2 participants