Address deepsource.io issues

[DimitriPapadopoulos]

Issues are from this page: https://deepsource.io/gh/DimitriPapadopoulos/yamllint/

I have left most raised issues out of the current merge request

• Not all issues are interesting, some might even be wrong.
• Would you agree to remove assert statements outside of tests?

  Usage of assert statement in application logic is discouraged. assert is removed with compiling to optimized byte code. Consider raising an exception instead. Ideally, assert statement should be used only in tests.

  Python has an option to compile the optimized bytecode and create the respective .pyo files by using the options -O and -OO. When used, these basic optimizations are done:

  • All the assert statements are removed
  • All docstrings are removed (when -OO is selected)
  • Value of the __debug__ built-in variable is set to False

  It is recommended not to use assert in non-test files. A better way for internal self-checks is to check explicitly and raise respective error using an if statement.

[adrienverge]

Hello Dimitri, thanks for these contributions!

1. I'm not sure about the first commit, is it worth complexifying the code, given that the generator here is well known (it comes from yamllint parser), and we know for sure it can't raise a StopIteration?

2. About the second commit: nice improvement! I propose a commit title like quoted-strings: Merge comparisons with 'in'; I try to keep "good commit titles" with scopes and imperative actions, for easier maintenance and Git usage.

3. For the third one, personnally I find the new version harder to understand. I need a few seconds to understand what it does and why, whereas it was a bit clearer before. What do you think?

Would you agree to remove assert statements outside of tests?

Yes, it makes sense! You would turn them into Exceptions?

[coveralls]

Coverage decreased (-0.2%) to 99.005% when pulling 632e218a580d3946d63689eeab9d6aee2e0b2dd5 on DimitriPapadopoulos:address_deepsource.io_issues into a09ad89268e9042349b764084426617da69957d3 on adrienverge:master.

[coveralls]

Coverage increased (+0.2%) to 99.178% when pulling 5d56eb49d51e0dd87269587a24b531a2f7e4ea84 on DimitriPapadopoulos:address_deepsource.io_issues into 008db4aa098b621cc50479d90f7f839751ae813a on adrienverge:master.

[DimitriPapadopoulos]

1. I agree: too much overhead to avoid an error that won't happen any way. I could ignore rules instead, with # skipcq: PTC-W0063 and perhaps a human-readable comment.
2. I have changed the commit title.
3. I don't know. Most languages do have a max() function in their library. If such a function is widely available, I suppose that's because most people find it more readable. But then you might dislike this specific case:

   value1 = max(value1, value2)
   

   but have nothing against the general case:

   new_value = max(value1, value2)
   

About assert and exceptions. I can change the asserts into exceptions, unless you think the asserts are used to catch corner cases during development and cannot be triggered at run-time in mature code. See: https://discuss.deepsource.io/t/using-assert-outside-tests/79/3

[adrienverge]

2. Thanks!
3. OK, let's do this then. Here also, can you update the commit title? (e.g. something like comments-indentation: Refactor to use 'max' builtin)

About assert and exceptions. I can change the asserts into exceptions, unless you think the asserts are used to catch corner cases during development and cannot be triggered at run-time in mature code.

asserts inside tests can be left alone, but asserts in the code (yamllint/) do mean "if this condition is true, stop execution now to avoid doing something bad". So if you want to turn them into exceptions, I'm :+1:

[DimitriPapadopoulos]

I think I have addressed all comments. I would rather defer assert fixes to another merge request.

[adrienverge]

Agreed.

Thank you @DimitriPapadopoulos!