Housekeeping Items

Questions and Help
#1

I’ve been trying to add a few more unit test cases to increase the existing coverage value and noticed a few things. This thread is to track them and check to see if they are something that needs to be addressed and get a feedback from the community.

Cookies

Max-Age

The current implementation of the Cookie class attempts to set the cookie as follows.

try:
    output.append("%s=%d" % (self._keys[key], value))
except TypeError:
    output.append("%s=%s" % (self._keys[key], value))

I don’t think we need to have this try-except. Instead, we might be better off with enforcing the value of Max-Age to be integers. As per the HTTP Cookie standards, this can’t be anything but an integer value.

Expires

As per the HTTP Cookie standards, the format of the Expires has to adhere to Date: <day-name>, <day> <month> <year> <hour>:<minute>:<second> GMT
In our case, if the value can’t be converted into that format, we return the text/value as is.

Request

repr

The following condition check in the repr hook for the Request class seems unnecessary.
How would one get either of these items to be an empty value?

if self.method is None or not self.path: 
    return "<{0}>".format(self.__class__.__name__)
#2

That all sounds reasonable. last one regarding repr, I would want to look more carefully at the code to draw the conclusion for myself,but it certainly seems impossible.

#3
  • Cookies
    • Max-Age: is it possible to pass an arbitrary value (read as “not an int”) to it?
    • Expires: I think that, if the value provided is not a datetime instance, we should raise an exception, for two reasons:
      1. How would we format any instance in that format if it is not a datetime?
      2. Even if it is a str, there’s no guarantee that it’ll be in the right format.
  • Request
    • __repr__: take a look at the source where it creates a Request instance. I don’t think method or path would ever be None. Also, take a look at the Request constructor itself. url_bytes and method are required arguments. Since path is a property, I would write the __repr__ method like this:
      def __repr__(self):
    return "<{0}: {1} {2}>".format(
        self.__class__.__name__, self.method, self.path
    )
1 Like
#4

  1. I think we should probably ensure that one doesn’t pass anything but a valid in to Max-Age based on its spec. By ensuring I mean if the value raises an exception with int(age) we default it to -1 or 0 and let it expire immediately. And raise a warning to inform the same

  2. I am with you on this one. Enforcing it to be datetime instance allows the framework to do a more pointed conversion with ease.

  3. Agreed. That’s a more concise and simpler implementation.

#5

Great, agreed! :metal:

#6

Let’s not lose sight of this. Bumping to keep it fresh.

  1. Enforce types for cookie values
  2. Request repr
#7

@ahopkins I have a PR with test adjustments on the way to address both the items.

#8

It also may be worthwhile to just open them as issues to mark as “beginner” to offer them for grabs since they are simple changes.

#9

That works too. I will create two new issues and mark them as a beginner and hold onto my PR.

Edit

I couldn’t tag the issues. Can you please tag the following issues in Git

  1. Request __repr__ fix
  2. Cookie Mag-Age
  3. Cookie Expires