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

Docstrings and expand tests #10

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Docstrings and expand tests #10

wants to merge 4 commits into from

Conversation

Cobord
Copy link

@Cobord Cobord commented Sep 11, 2024

No description provided.

Copy link

google-cla bot commented Sep 11, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Collaborator

@NoureldinYosri NoureldinYosri 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 offering to help. test files don't usually have docstrings, tests should be simple and readable. they can have simple comments but if they require extensive comments or docstrings then they need to be simplified which is not the case here.



@pytest.mark.parametrize('dimension', _ALL_DIMENSIONS)
def test_arithmetic_ops_preserve_type_multi_array(
Copy link
Collaborator

Choose a reason for hiding this comment

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

from TypedUnits perspective there is no difference between a 1-D array and a multidimensional, both are a numpy.ndarray so this case is implicitly covered by the tests.

We can consider explicitly testing for this though but for that please improve the test.


c = a + b
should_all_true = c == u * [[7.0, 10.0], [12.0, 14.0]]
assert should_all_true.all().all()
Copy link
Collaborator

Choose a reason for hiding this comment

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

a single .all() is enough

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