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

Removed unused __str__() methods in tests models. #12305

Merged
merged 1 commit into from Apr 15, 2020

Conversation

atombrella
Copy link
Contributor

Maybe there are more of such things in the test suite. Perhaps we can leave this PR to look for more, instead of merging immediately.

@adamchainz
Copy link
Sponsor Member

How did you find these? Ran the test suite with coverage? I see our Jenkins coverage report doesn't cover the tests.

@atombrella
Copy link
Contributor Author

atombrella commented Jan 18, 2020

How did you find these? Ran the test suite with coverage? I see our Jenkins coverage report doesn't cover the tests.

It was a bit by coincident: I opened a models file in PyCharm, and the self.name was highlighted. Then I thought that they're probably more. The content of some of those models file has not been touched for ~8-12 years. I haven't thought of coverage of the tests.

@hramezani
Copy link
Member

@atombrella I found some of them. it is possible to push to your branch?

@felixxm
Copy link
Member

felixxm commented Jan 22, 2020

I pushed few more.

@felixxm
Copy link
Member

felixxm commented Jan 22, 2020

... and 7 more. 457 __str__() methods left.

@atombrella
Copy link
Contributor Author

atombrella commented Jan 22, 2020

@atombrella I found some of them. it is possible to push to your branch?

@hramezani Thank you, please just list them here. I think that's a better approach. @felixxm Please remember to add yourself in Co-Authored-By. Thank you.

It seems that self.name caught quite a few of them.

Do you use something like grep -r -A 2 -B 10 __str__ --include="*.py" tests/ to check?

@hramezani
Copy link
Member

Do you use something like grep -r -A 2 -B 10 str --include="*.py" tests/ to check?

Yes, I do.

Here is the diff file. It will remove 25 of them.

@felixxm
Copy link
Member

felixxm commented Jan 22, 2020

I used find -name models.py | xargs grep 'def __str__' .

@felixxm felixxm changed the title Removed an unused __str__ method in test.validation.Post. Removed unused __str__ methods in tests models. Jan 22, 2020
@atombrella atombrella force-pushed the unused_model_str_tests branch 2 times, most recently from 0fd0bea to 89f57f4 Compare February 10, 2020 14:57
Co-Authored-By: Mariusz Felisiak <felisiak.mariusz@gmail.com>
Co-Authored-By: Hasan Ramezani <hasan.r67@gmail.com>
@felixxm
Copy link
Member

felixxm commented Apr 15, 2020

I added few more. I think it's a good start that it's worth to merge. We removed 50 methods.

@felixxm felixxm changed the title Removed unused __str__ methods in tests models. Removed unused __str__() methods in tests models. Apr 15, 2020
@felixxm felixxm merged commit 6461583 into django:master Apr 15, 2020
@atombrella atombrella deleted the unused_model_str_tests branch April 15, 2020 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants