Cobular / distest

A library used to do integration testing on discord bots
MIT License
29 stars 8 forks source link

Add Code Factor for basic code quality review #8

Closed Cobular closed 5 years ago

Cobular commented 5 years ago

Hey all, I'm looking into adding https://www.codefactor.io/ to the project in order to try to catch the most basic errors and keep the quality high.

EDIT: This is (for now) the place to talk about code quality changes that aren't severe enough to merit their own issue. If it will just be 2 or 3 comments, just have it here rather than in a brand new issue.

JosephFKnight commented 5 years ago

That's a good idea. and codefactor actually found an issue that is preventing me from refactoring further: the global EXIT_CODE variable. globals are almost always bad practice and we'll need to get rid of it.

If you explain a bit your rationale for having it in there, I will likely be able to figure out a way to do without it. It's for the CLI mode, yes? So that the program exits with RC 1 if there is a test failure?

Cobular commented 5 years ago

Basically, It's what you noted. I know it's bad practice, and it's sort of a half-assed solution, but I needed a way to set EXIT_CODE at the file level so that it could be exited. See this code snippet: https://github.com/JakeCover/distest/blob/ce78a862c2e80095423a250edfdd034cb0cbb6e1/distest/__init__.py#L393-L399 I have found that nothing in that function will run after I call client.close(), so I couldn't use sys.exit(1) or a return statement, since that would cause either the exit()/return or close() to not fire. (Does that make sense?) The next place my code runs is right after bot.run() in the following snippet: https://github.com/JakeCover/distest/blob/ce78a862c2e80095423a250edfdd034cb0cbb6e1/distest/__init__.py#L697-L700 This means that the exit code has to be processed there, if everything else stays the same. I really don't like this solution, as it uses globals, but it's the first thing me and some people on the disord.py server thought of when I brought it up.

EDIT: After writing this, I looked at the PRs and I see that you fixed it in 5bb0c51. That is a much more elegant way to do this! Thank you!