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

Warn when generating overly large repr #3848

Merged
merged 8 commits into from
Jan 17, 2024

Conversation

jobh
Copy link
Contributor

@jobh jobh commented Jan 16, 2024

For consideration, and submitted for the test runs.

A warning like this would have saved some development time lately, on #3820 and #3837.

@jobh jobh marked this pull request as ready for review January 16, 2024 15:01
data._sampled_from_all_strategies_elements_message = (
"sample_from was given a collection of strategies: "
f"{preamble}{strat_reprs}. Was one_of intended?"
"{!r}. Was one_of intended?", self.elements
Copy link
Contributor Author

@jobh jobh Jan 16, 2024

Choose a reason for hiding this comment

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

The warning triggered on this code - on win only, for some reason - so I delayed the repr until we actually add the note.

Since it caused a test failure, I propose that this does not violate the one-purpose-PR rule ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note - max 3 is removed, since I think it was only added as a workaround for this specific problem in the first place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(not really outdated, I just rebased for the RELEASE.rst conflict)

@jobh jobh force-pushed the warn-construct-overly-long-repr branch from d8cdbd6 to 0128ab1 Compare January 16, 2024 15:16
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Thanks @jobh! I'm not sure whether to hope that we'll never need this, or that it'll save our bacon someday - but I'm very glad to have it.

I just don't understand why the coverage job is claiming it's missed a line, when I can see that test_define_function_signature_validates_function_name does execute it!

@jobh
Copy link
Contributor Author

jobh commented Jan 17, 2024

I just don't understand why the coverage job is claiming it's missed a line, when I can see that test_define_function_signature_validates_function_name does execute it!

I guess the new test should be moved to tests/cover/, will do.

@Zac-HD
Copy link
Member

Zac-HD commented Jan 17, 2024

That's just it though, the coverage problem is for previously-existing code, and the test is already in tests/cover/test_reflection.py 😕

@jobh
Copy link
Contributor Author

jobh commented Jan 17, 2024

That's just it though, the coverage problem is for previously-existing code,

No, it's line 485 and that's

        warnings.warn(
            "Generating overly large repr. This is an expensive operation, and with "

I'll move the test quickly

@Zac-HD
Copy link
Member

Zac-HD commented Jan 17, 2024

🤦 I must have had the wrong branch checked out, oops!

@Zac-HD Zac-HD merged commit 032c06d into HypothesisWorks:master Jan 17, 2024
@jobh jobh deleted the warn-construct-overly-long-repr branch January 17, 2024 12:52
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