stem icon indicating copy to clipboard operation
stem copied to clipboard

Add `is_reasonably_live` property for `NetworkStatusDocument`

Open jbrown299 opened this issue 4 years ago • 1 comments

Inside NetworkStatusDocumentV3 you have is_fresh method:

  def is_fresh(self):
    """
    Checks if the current time is between this document's **valid_after** and
    **fresh_until** timestamps. To be fresh means this should be the latest
    consensus.

    .. versionadded:: 1.8.0

    :returns: **True** if this consensus is presently fresh and **False**
      otherwise
    """

    return self.valid_after < datetime.datetime.utcnow() < self.fresh_until
  1. May be better to define it as property.
  2. May be better follow original tor source client which naming it as networkstatus_is_live.
  3. We need second propery is_reasonably_live like networkstatus_consensus_reasonably_live. It is used when tor client try use old consensus but not so old.
  4. Need to double check is it really < and > or <= and >=

From torpy sources:

    @property
    def is_live(self):
        # tor ref: networkstatus_is_live
        return self.valid_after <= datetime.utcnow() <= self.valid_until

    @property
    def is_reasonably_live(self):
        # tor ref: networkstatus_consensus_reasonably_live
        return self.valid_after - timedelta(hours=24) <= datetime.utcnow() <= self.valid_until + timedelta(hours=24)

jbrown299 avatar Jan 17 '21 13:01 jbrown299

May be better to define it as property.

Hi James. This is an interesting idea but I'm torn. The rule of thumb I use is 'property for static values, method for dynamic'. Personally I'd find it confusing to have our object's is_live attribute be True now, and False five minutes from now.

Methods by contrast are clearly evaluated at runtime, fulfilling the python axium Explicit is better than implicit..

Thoughts?

atagar avatar Jan 17 '21 23:01 atagar