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

added missing test #205

Merged
merged 5 commits into from
May 2, 2024
Merged

added missing test #205

merged 5 commits into from
May 2, 2024

Conversation

j042
Copy link
Member

@j042 j042 commented Apr 23, 2024

helper classes didn't have tests. This one adds one for data.py. Related to #202

@j042 j042 requested review from clemens-fricke and jzwar April 23, 2024 13:01
@j042 j042 mentioned this pull request Apr 23, 2024
@j042 j042 changed the base branch from main to ft-add-update April 23, 2024 13:14
@j042 j042 force-pushed the ft-helper-tests branch from 097edf7 to fba6ee2 Compare April 23, 2024 13:15
@j042 j042 force-pushed the ft-helper-tests branch from fba6ee2 to b41c1e4 Compare April 23, 2024 13:16
Copy link
Collaborator

@clemens-fricke clemens-fricke left a comment

Choose a reason for hiding this comment

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

Looks good.

Comment on lines +150 to +158
assert dataholder.get("a") == 1
# 2. key and default
assert dataholder.get("b", 2) == 2
# 3. key and wrong default
assert dataholder.get("b", 3) == 2
# 4. empty key - always None
assert dataholder.get("c") is None
# 5. empty key and default
assert dataholder.get("c", "123") == "123"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we have gotten this functionality through inheritance from a build-in class, like default dict?

Copy link
Member Author

Choose a reason for hiding this comment

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

could, yes, I just didn't wanna inherit from the built-in classes

Copy link
Collaborator

@jzwar jzwar left a comment

Choose a reason for hiding this comment

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

Lgtm


ta = new_tracked_array()
ta /= 1.5
assert ta.modified
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would feel better if before the modification it had assert not ta.modified at least once

Copy link
Collaborator

@clemens-fricke clemens-fricke Apr 24, 2024

Choose a reason for hiding this comment

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

That is in the new_tracked_array() function. At least he sets it to False in the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you

assert value is not func()


@pytest.mark.parametrize(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, ...

@j042 j042 merged commit 1987587 into ft-add-update May 2, 2024
2 checks passed
@j042 j042 deleted the ft-helper-tests branch May 2, 2024 18:30
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.

3 participants