need moved the check parameter to the top of the isnstance check, because if bs = None then an exception is thrown

Crash is typically a segfault, not an exception.

Please describe the problem you would like to see fixed, rather than the code change you would like to make.

Irit, the problem is that if you pass the bs parameter equal to None to the quote_from_bytes function, then the bs parameter will not be checked for existence, because a TypeError exception will be raised, and an empty string needs to be returned in this case. The check "if not bs" should be earlier than "if not isinstance (bs, (bytes, bytearray))", because "if bs = None", then the code will not reach this check

Right, so the question is whether it is correct that "an empty string needs to be returned in this case". Adding orsenthil to answer that as the module's expert.

Also, your patch would need a unit test.

Clarifying the title.

If you want to interpret None as an empty string, you can just write quote_from_bytes(data or b'').

That is, all the libraries already created need to follow your advice? :)

I don’t know if it needs to change, but if anything I would map None to None and not to ‘’.

What are the next actions? Do I need to do something or are we waiting for something?

As I wrote on the PR, and again on this issue, a PR that makes a behavior change (like this one) is not complete without a unit test (which fails before the change and passes after the change, and can show what the bug being fixed is).  You could add such a test.

In this case, it is also not clear that the current behavior is a bug at all, and if it is then what the fix should be (you propose to map None to ''. But why not 'None'? Why not 'Mary had a little lamb'?  I suggested to return None rather than some arbitrary string). You could explain why you think it's a bug and why you think '' is the correct return value.

Once you do write the test and there is consensus that it is a bug and we agree what the fix should be, it should be some core dev's top priority to review and merge the PR (as opposed to reviewing and merging another PR). So you could push it forward by explaining why this bug is important.

The documentation states that this function accepts bytes:

https://docs.python.org/3/library/urllib.parse.html#urllib.parse.quote_from_bytes

None is not of type bytes, so raising a TypeError is not unreasonable. It would certainly be wrong to return any string. It could have returned None but that doesn't make a massive usability difference and it's not worth changing now.

I propose to close this as 'not a bug'.

If you pass None to the quote_from_bytes function, then there is no point in the "if not bs" check, because it won't even reach it. 

This function is not with dynamic behavior, which violates python concepts. If you pass a string instead of bytes, it will throw a TypeError exception, it's ok. But if for some reason you need to pass None, and this happens, then the function does not behave as expected.

Why even check that bs is not None, if this can never be? And if it does, there will always be a TypeError exception.

There is still a point in the "if not bs:" check, it will be true for bs which is an empty string.

I meant:

... it will be true for bs which is an empty bytes().


You are thinking of b'' and None as if they are the same thing. They are not.  If this was a check for None it would be "if bs is None" and not "if not bs".

I agree this should be closed (and I'm closing it). I don't see any reason why this function should do something other than raise TypeError if given bs=None. If you want that behavior, write a small wrapper function.

The "if not bs" check appears to be an optimization for the case of zero-length input. Hopefully the code would continue to work without that test (or instead testing for len(bs)==0), but in my opinion it's not worth the risk of removing or changing it.