temporal: Fixed E722 in temporal_granularity

[arohanajit]

Add specific exceptions for E722 in temporal_granularity.py i.e ValueError. Didn't add TypeError for None objects which seemed less likely to occur via code error.

[echoix]

@petrasovaa I'm wondering if restricting the types of errors caught here is really helpful for that specific function.

It is a function that is used to validate if the granularity string is valid or not. So, you take a large domain of possible argument values (and types), and narrow down to True or False, True being when it is valid. By restricting the error types of the try-catch, it means we would fall back to python's exceptions and tracebacks if there's some sort of errors that aren't handled. In normal cases, it wouldn't bother much, but here, the function is written to fallback to true, and returning false on selected errors, rather than being written to return true only when it is really valid. With this PRs, other errors that would have returned true would now either return true or generate an error here or somewhere else in the code.

[echoix]

Also the test for relative time is only checking if the argument is convertible to int, not that it is an int precisely. int() will accept a string of integers (including negatives), or a float, but not a string of float. It will however accept a string of integers with leading spaces and trailing newline, as well as accept the underscore inside the number as used like a number litteral grouping symbol, and also valid hexadecimal, prefixed by 0x or not (like "FACE" or "0xface" which both represent 64206). https://docs.python.org/3/library/functions.html#int

Is it what we expect here? Is that temporal granularity in relative time passed to a C function that wouldn't consider it valid?

[arohanajit]

@petrasovaa I'm wondering if restricting the types of errors caught here is really helpful for that specific function.

It is a function that is used to validate if the granularity string is valid or not. So, you take a large domain of possible argument values (and types), and narrow down to True or False, True being when it is valid. By restricting the error types of the try-catch, it means we would fall back to python's exceptions and tracebacks if there's some sort of errors that aren't handled. In normal cases, it wouldn't bother much, but here, the function is written to fallback to true, and returning false on selected errors, rather than being written to return true only when it is really valid. With this PRs, other errors that would have returned true would now either return true or generate an error here or somewhere else in the code.

Good point. Since this is a validation function, it makes more sense to catch all exceptions and return False for any invalid input rather than letting some exceptions propagate. I could only find potential ValueError that could occur. In that case I can also use general Exception here

[arohanajit]

This issue is addressed in #3748 , is this separate PR reqd or do I close this?

[echoix]

This issue is addressed in #3748 , is this separate PR reqd or do I close this?

#3748 is (or was initially) kinda big, and doesn't seem close to be finished if the trend continues. I don't think the author is active to continue adjusting it (it was for her job). This PR here is easier and faster to review and merge, so we might as well finish it off here

[petrasovaa]

@petrasovaa I'm wondering if restricting the types of errors caught here is really helpful for that specific function.
It is a function that is used to validate if the granularity string is valid or not. So, you take a large domain of possible argument values (and types), and narrow down to True or False, True being when it is valid. By restricting the error types of the try-catch, it means we would fall back to python's exceptions and tracebacks if there's some sort of errors that aren't handled. In normal cases, it wouldn't bother much, but here, the function is written to fallback to true, and returning false on selected errors, rather than being written to return true only when it is really valid. With this PRs, other errors that would have returned true would now either return true or generate an error here or somewhere else in the code.

Good point. Since this is a validation function, it makes more sense to catch all exceptions and return False for any invalid input rather than letting some exceptions propagate. I could only find potential ValueError that could occur. In that case I can also use general Exception here

I see your point, but realistically, there is not that much that can happen here, I would add the missing AttributeError there and move on.