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

Replace and rewrite "assert" statements in non-test code #175

Open
pombredanne opened this issue Jan 12, 2025 · 11 comments
Open

Replace and rewrite "assert" statements in non-test code #175

pombredanne opened this issue Jan 12, 2025 · 11 comments

Comments

@pombredanne
Copy link
Member

See https://www.reddit.com/r/learnpython/comments/zd217y/is_using_assert_considered_good_practice/

assert statements should only be used for debugging purposes. They are removed when not running in debug mode (i.e. when invoking the Python command with the -O or -OO options).

Instead use a raise statement to throw a proper, informative runtime exception, if you need to. That would definitely be best practice when there'd otherwise be no sensible return value for the function.

We are using a few asserts in our main code. See https://github.com/search?q=org%3Aaboutcode-org+NOT+path%3A*test*++"assert"++language%3APython&type=code

@pombredanne pombredanne transferred this issue from aboutcode-org/scancode-toolkit Jan 12, 2025
@TirthPanchal
Copy link

Heyyy, I'd like to work on this issue. Can you assign it to me?

@coderhuBypassion
Copy link

so, assert statements should be removed from all the repos of aboutcode-org., or only from aboutcode-org/scancode-toolkit ?

@Dhruv276R
Copy link

Dhruv276R commented Jan 14, 2025

Hey, I'd like to work on this issue. Can you assign this to me? And the assert statements are to be removed from all the repos or a specific one? @pombredanne

@Gurleen-kansray
Copy link

Hi @pombredanne , I have started working on this issue. I will replace the assert statements with raise statements in the main code (outside test files). I'll submit a PR soon. Let me know if there are specific repos other than scancode-toolkit where this change is needed.

@Dhruv276R
Copy link

Dhruv276R commented Jan 15, 2025

@pombredanne I have made a PR on this https://github.com/aboutcode-org/aboutcode-toolkit/pull/574
Check and let me know for further enhacements. Do you need me to changes in other repos as well?

@ONKARBH
Copy link

ONKARBH commented Jan 17, 2025

Hi @pombredanne, I would like to work on this issue to replace the assert statements with proper raise exceptions in the main code (outside test files). Could you please assign this issue to me? Let me know if any specific guidelines or repos need to be considered. Thank you!

@pombredanne
Copy link
Member Author

@TirthPanchal re:

Heyyy, I'd like to work on this issue. Can you assign it to me?

we do not "assign" issue per se. You are welcome to work on this, but you want to sync up here with others to avoid double, duplicated work!

@Dhruv276R re:

Hey, I'd like to work on this issue. Can you assign this to me?

As explained above, we cannot "assign" work. You guys need to coordinate and collaborate to avoid stepping on each other.

And the assert statements are to be removed from all the repos or a specific one?

Rather replaced in all the repos, and only asserts that are not for tests, and only asserts that are for production code that could be ignored from a -O python run.

@Gurleen-kansray re:

Hi , I have started working on this issue. I will replace the assert statements with raise statements in the main code (outside test files). I'll submit a PR soon. Let me know if there are specific repos other than scancode-toolkit where this change is needed.

As said above, all repos, but not in test code: there we use "assert" and pytest does an awesome job assert rewriting job, so we surely do not want to change this.

@ONKARBH re:

Hi, I would like to work on this issue to replace the assert statements with proper raise exceptions in the main code (outside test files). Could you please assign this issue to me? Let me know if any specific guidelines or repos need to be considered. Thank you!

Same as above: all repos, not the tests, and you need to coordinate here with others

@ONKARBH
Copy link

ONKARBH commented Jan 17, 2025

Hi everyone, I’d like to contribute to this issue by focusing on specific files/modules for replacing assert statements outside the test files. Please let me know how I can best coordinate with others to avoid overlap. Thank you!

@pombredanne
Copy link
Member Author

@Dhruv276R re:

I have made a PR on this https://github.com/aboutcode-org/aboutcode-toolkit/pull/574 Check and let me know for further enhacements. Do you need me to changes in other repos as well?

This is awesome, but I cannot see the PR at https://github.com/aboutcode-org/aboutcode/pulls/574 ?

@ONKARBH
Copy link

ONKARBH commented Jan 17, 2025

Hi @Dhruv276R, your work is great! However, the PR link seems incorrect. Can you confirm if the PR is in the aboutcode-toolkit repo and share the correct link? Thank you!

@pombredanne
Copy link
Member Author

My bad: this is at aboutcode-org/aboutcode-toolkit#574

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants