I nearly expounded on this in the log message, but decided that a
regular email to the list is a much better approach. Specifically to
talk about respect for your fellow developers.
When Jane vetoes John's change, then a discussion needs to be started
about the change and what kind of resolution is warranted. Jane does
NOT immediately revert John's change. He may be right after all, or
maybe with some tweaks, the technical problem can be corrected. This
is about showing respect to John to determine the final course of
action. He will let his change stand (Jane removes her veto after some
discussion), he can tweak it (some compromise is reached after
discussion), or he can revert it when he realizes that his fellow
developers disagree with his change and no compromise solution looks
to be available. Jane gives hoim the respect, the time, and the
discussion to take the appropriate course of action.
However, when that third option is reached: the veto stands, even
after discussion, then John MUST respect that decision. This is a
community of peers, and we need to show respect to the opinions,
decisions, and approaches of others. That also means vetoes should not
be casually thrown around: it is a very harsh measure of your peer's
work. In many cases, you may want to consider whether John's change is
"good enough" and just let it stand. Or provide some review on how it
could be improved. Pulling out the veto card is divisive, so Jane
needs to really think about whether the technical problems introduced
by the change warrant her veto.
And with that said, I didn't see the two vetoed revisions getting
backed out as they should have. I wanted to let the original developer
do it, but he was not respecting mine and Branko's issues on the
matter. So with reluctance, I had to do it myself.
Then write this email for why I feel that was wrong.
-g
On Tue, Mar 31, 2009 at 17:24, Greg Stein <gstein_at_gmail.com> wrote:
> Author: gstein
> Date: Tue Mar 31 08:24:16 2009
> New Revision: 36895
>
> Log:
> Revert r36822 and r36833, due to vetoes.
>
> * subversion/bindings/swig/python/svn/core.py:
> (svn_path_compare_paths): restore use of cmp()
>
> * subversion/tests/cmdline/svntest/tree.py:
> (SVNTreeNode.__cmp__): restored
> (SVNTreeNode.__eq__, SVNTreeNode.__ne__): removed
>
> * subversion/tests/cmdline/svntest/verify.py:
> (ExpectedOutput.__cmp__): restored
> (ExpectedOutput.__eq__, ExpectedOutput.__ne__): removed
> (UnorderedOutput.__cmp__): restored
>
> * tools/dev/contribulyze.py:
> (Contributor): restored (admittedly funky) hash behavior and __cmp__
> and __hash__ methods. removed __eq__, __ne__, __lt__, and __gt__
> methods.
> (LogMessage): restored __cmp__ and __hash__ methods. removed __eq__,
> __ne__, __lt__ and __gt__ methods.
>
> * tools/dev/trails.py:
> (output_summary): restored use of cmp() function
>
> Modified:
> trunk/subversion/bindings/swig/python/svn/core.py
> trunk/subversion/tests/cmdline/svntest/tree.py
> trunk/subversion/tests/cmdline/svntest/verify.py
> trunk/tools/dev/contribulyze.py
> trunk/tools/dev/trails.py
>
> Modified: trunk/subversion/bindings/swig/python/svn/core.py
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/bindings/swig/python/svn/core.py?pathrev=36895&r1=36894&r2=36895
> ==============================================================================
> --- trunk/subversion/bindings/swig/python/svn/core.py Tue Mar 31 07:51:57 2009 (r36894)
> +++ trunk/subversion/bindings/swig/python/svn/core.py Tue Mar 31 08:24:16 2009 (r36895)
> @@ -121,7 +121,7 @@ def svn_path_compare_paths(path1, path2)
>
> # Common prefix was skipped above, next character is compared to
> # determine order
> - return (char1 > char2) - (char1 < char2)
> + return cmp(char1, char2)
>
> def svn_mergeinfo_merge(mergeinfo, changes):
> return _libsvncore.svn_swig_mergeinfo_merge(mergeinfo, changes)
>
> Modified: trunk/subversion/tests/cmdline/svntest/tree.py
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/tests/cmdline/svntest/tree.py?pathrev=36895&r1=36894&r2=36895
> ==============================================================================
> --- trunk/subversion/tests/cmdline/svntest/tree.py Tue Mar 31 07:51:57 2009 (r36894)
> +++ trunk/subversion/tests/cmdline/svntest/tree.py Tue Mar 31 08:24:16 2009 (r36895)
> @@ -276,11 +276,11 @@ class SVNTreeNode:
> return s.getvalue()
>
>
> - def __eq__(self, other):
> - return self.name == other.name
> -
> - def __ne__(self, other):
> - return not self.__eq__(other)
> + def __cmp__(self, other):
> + """Define a simple ordering of two nodes without regard to their full
> + path (i.e. position in the tree). This can be used for sorting the
> + children within a directory."""
> + return cmp(self.name, other.name)
>
> def as_state(self, prefix=None):
> root = self
>
> Modified: trunk/subversion/tests/cmdline/svntest/verify.py
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/tests/cmdline/svntest/verify.py?pathrev=36895&r1=36894&r2=36895
> ==============================================================================
> --- trunk/subversion/tests/cmdline/svntest/verify.py Tue Mar 31 07:51:57 2009 (r36894)
> +++ trunk/subversion/tests/cmdline/svntest/verify.py Tue Mar 31 08:24:16 2009 (r36895)
> @@ -93,7 +93,7 @@ class ExpectedOutput:
> def __str__(self):
> return str(self.output)
>
> - def __eq__(self, other):
> + def __cmp__(self, other):
> """Return whether SELF.output matches OTHER (which may be a list
> of newline-terminated lines, or a single string). Either value
> may be None."""
> @@ -116,12 +116,9 @@ class ExpectedOutput:
> is_match = False
>
> if is_match:
> - return True
> + return 0
> else:
> - return False
> -
> - def __ne__(self, other):
> - return not self.__eq__(other)
> + return 1
>
> def is_equivalent_list(self, expected, actual):
> "Return whether EXPECTED and ACTUAL are equivalent."
> @@ -219,6 +216,14 @@ class RegexOutput(ExpectedOutput):
>
> class UnorderedOutput(ExpectedOutput):
> """Marks unordered output, and performs comparisions."""
> +
> + def __cmp__(self, other):
> + "Handle ValueError."
> + try:
> + return ExpectedOutput.__cmp__(self, other)
> + except ValueError:
> + return 1
> +
> def is_equivalent_list(self, expected, actual):
> "Disregard the order of ACTUAL lines during comparison."
> if self.match_all:
>
> Modified: trunk/tools/dev/contribulyze.py
> URL: http://svn.collab.net/viewvc/svn/trunk/tools/dev/contribulyze.py?pathrev=36895&r1=36894&r2=36895
> ==============================================================================
> --- trunk/tools/dev/contribulyze.py Tue Mar 31 07:51:57 2009 (r36894)
> +++ trunk/tools/dev/contribulyze.py Tue Mar 31 08:24:16 2009 (r36895)
> @@ -122,7 +122,7 @@ def html_footer():
> return '\n</body>\n</html>\n'
>
>
> -class Contributor(object):
> +class Contributor:
> # Map contributor names to contributor instances, so that there
> # exists exactly one instance associated with a given name.
> # Fold names with email addresses. That is, if we see someone
> @@ -131,6 +131,9 @@ class Contributor(object):
> # instance, and store it under both the email and the real name.
> all_contributors = { }
>
> + # See __hash__() for why this is necessary.
> + hash_value = 1
> +
> def __init__(self, username, real_name, email):
> """Instantiate a contributor. Don't use this to generate a
> Contributor for an external caller, though, use .get() instead."""
> @@ -144,6 +147,9 @@ class Contributor(object):
> # "Patch" represent all the revisions for which this contributor
> # contributed a patch.
> self.activities = { }
> + # Sigh.
> + self.unique_hash_value = Contributor.hash_value
> + Contributor.hash_value += 1
>
> def add_activity(self, field_name, log):
> """Record that this contributor was active in FIELD_NAME in LOG."""
> @@ -224,17 +230,20 @@ class Contributor(object):
> else:
> return other_str
>
> - def __eq__(self, other):
> - return self.score() == other.score() and self.big_name() == other.big_name()
> -
> - def __ne__(self, other):
> - return not self.__eq__(other)
> -
> - def __lt__(self, other):
> - return self.score() > other.score() or (self.score() == other.score() and self.big_name() < other.big_name())
> + def __cmp__(self, other):
> + if self.is_full_committer and not other.is_full_committer:
> + return 1
> + if other.is_full_committer and not self.is_full_committer:
> + return -1
> + result = cmp(self.score(), other.score())
> + if result == 0:
> + return cmp(self.big_name(), other.big_name())
> + else:
> + return 0 - result
>
> - def __gt__(self, other):
> - return self.score() < other.score() or (self.score() == other.score() and self.big_name() > other.big_name())
> + def __hash__(self):
> + """See LogMessage.__hash__() for why this exists."""
> + return self.hash_value
>
> @staticmethod
> def parse(name):
> @@ -431,7 +440,7 @@ class Field:
> return s
>
>
> -class LogMessage(object):
> +class LogMessage:
> # Maps revision strings (e.g., "r12345") onto LogMessage instances,
> # holding all the LogMessage instances ever created.
> all_logs = { }
> @@ -459,17 +468,28 @@ class LogMessage(object):
> """Accumulate one more line of raw message."""
> self.message += line
>
> - def __eq__(self, other):
> - return int(self.revision[1:]) == int(other.revision[1:])
> -
> - def __ne__(self, other):
> - return not self.__eq__(other)
> -
> - def __lt__(self, other):
> - return int(self.revision[1:]) > int(other.revision[1:])
> + def __cmp__(self, other):
> + """Compare two log messages by revision number, for sort().
> + Return -1, 0 or 1 depending on whether a > b, a == b, or a < b.
> + Note that this is reversed from normal sorting behavior, but it's
> + what we want for reverse chronological ordering of revisions."""
> + a = int(self.revision[1:])
> + b = int(other.revision[1:])
> + if a > b: return -1
> + if a < b: return 1
> + else: return 0
> +
> + def __hash__(self):
> + """I don't really understand why defining __cmp__() but not
> + __hash__() renders an object type unfit to be a dictionary key,
> + especially in light of the recommendation that if a class defines
> + mutable objects and implements __cmp__() or __eq__(), then it
> + should not implement __hash__(). See these for details:
> + http://mail.python.org/pipermail/python-dev/2004-February/042580.html
> + http://mail.python.org/pipermail/python-bugs-list/2003-December/021314.html
>
> - def __gt__(self, other):
> - return int(self.revision[1:]) < int(other.revision[1:])
> + In the meantime, I think it's safe to use the revision as a hash value."""
> + return int(self.revision[1:])
>
> def __str__(self):
> s = '=' * 15
>
> Modified: trunk/tools/dev/trails.py
> URL: http://svn.collab.net/viewvc/svn/trunk/tools/dev/trails.py?pathrev=36895&r1=36894&r2=36895
> ==============================================================================
> --- trunk/tools/dev/trails.py Tue Mar 31 07:51:57 2009 (r36894)
> +++ trunk/tools/dev/trails.py Tue Mar 31 08:24:16 2009 (r36895)
> @@ -74,9 +74,9 @@ def output_summary(trails, outfile):
> def _freqtable_cmp(a_b, c_d):
> (a, b) = a_b
> (c, d) = c_d
> - c = (d > b) - (d < b)
> + c = cmp(d, b)
> if not c:
> - c = (a > c) - (a < c)
> + c = cmp(a, c)
> return c
>
> def list_frequencies(list):
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=1496768
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1496987
Received on 2009-03-31 17:49:19 CEST