Extend SymPyDeprecationWarning with additional information. (Issue 2142) #1065

Merged
merged 11 commits into from Apr 13, 2012

Projects

None yet

5 participants

@scolobb
Contributor
scolobb commented Feb 16, 2012

SymPyDeprecationWarning now offers convenient facilities for specifying the deprecated feature, the version in which the feature is expected to be removed and what the user should use instead of the deprecated feature.

@vperic vperic and 2 others commented on an outdated diff Feb 16, 2012
sympy/core/compatibility.py
@@ -4,11 +4,82 @@
"""
class SymPyDeprecationWarning(DeprecationWarning):
- def __init__(self, value):
- self.parameter = value
+ """A deprecation warning which can also hold the information about
+ the feature to be removed, when it will be removed, and what the
+ developers recommend to use instead.
+
+ This class is expected to be used with the warnings.warn function
+ (note that one has to explicitly turn on deprecation warnings):
@vperic
vperic Feb 16, 2012 Contributor

I believe that SymPyDeprecationWarning is turned on by default (one of the reasons we wanted to do this in the first place)... ?

@asmeurer
asmeurer Feb 17, 2012 Member

It should be turned on by default in isympy, but otherwise, it will use the default Python behavior (off by default in 2.6+ if I remember correctly).

@scolobb
scolobb Feb 17, 2012 Contributor

Yes, the warning does not show with default settings under the python interpreter.

@vperic vperic commented on an outdated diff Feb 16, 2012
sympy/core/compatibility.py
@@ -4,11 +4,82 @@
"""
class SymPyDeprecationWarning(DeprecationWarning):
- def __init__(self, value):
- self.parameter = value
+ """A deprecation warning which can also hold the information about
+ the feature to be removed, when it will be removed, and what the
+ developers recommend to use instead.
+
+ This class is expected to be used with the warnings.warn function
+ (note that one has to explicitly turn on deprecation warnings):
+
+ >>> from warnings import warn
+ >>> from sympy.core.compatibility import SymPyDeprecationWarning
+ >>> warnings.warn(\"Don't do this, it's deprecated\", SymPyDeprecationWarning)
@vperic
vperic Feb 16, 2012 Contributor

To avoid all the extra backslashes for every quote, you can declare the docstring as "raw".. it should work fine. (just use r"""blablabla""")

@Krastanov Krastanov and 2 others commented on an outdated diff Feb 16, 2012
sympy/core/compatibility.py
@@ -4,11 +4,82 @@
"""
class SymPyDeprecationWarning(DeprecationWarning):
- def __init__(self, value):
- self.parameter = value
+ """A deprecation warning which can also hold the information about
+ the feature to be removed, when it will be removed, and what the
+ developers recommend to use instead.
+
+ This class is expected to be used with the warnings.warn function
+ (note that one has to explicitly turn on deprecation warnings):
+
+ >>> from warnings import warn
+ >>> from sympy.core.compatibility import SymPyDeprecationWarning
+ >>> warnings.warn(\"Don't do this, it's deprecated\", SymPyDeprecationWarning)
+ __main__:1: SymPyDeprecationWarning: \"Don't do this, it's deprecated\"
@Krastanov
Krastanov Feb 16, 2012 Member

Have you checked these doctests. I am not sure that they would work on all platforms.

@scolobb
scolobb Feb 16, 2012 Contributor

I'm sorry, I only remembered to run the tests after I had sent the pull request :-( I've run the tests now, they fail.

I'll fix the things when I get up.

I'll try not to forget to run the tests any more.

@scolobb
scolobb Feb 17, 2012 Contributor

I cannot get the doctests to pass, due to this problem: http://pastebin.com/atJXPL1b . Does anybody have ideas on how to fix this?

@Krastanov
Krastanov Feb 17, 2012 Member

I suppose that in this case you can just skip the doctest (using #DOCTEST +SKIP or something like that, I do not remember the exact string). If the others find it appropriate you can add a test (not a doctest) using raises and setting all warnings to errors.

Also you may find gist to be a superior alternative to pastebin (check the link at the top of the page).

@scolobb
scolobb Feb 20, 2012 Contributor

Thank you for your advise! I took the way of skipping the doctest altogether, instead of trying to get it pass. The reason is that, according to what is written here, one should either turn warnings on explicitly with simplefilter, which I did and it didn't work for me, or one should write a decorator. I chose to consider that writing so much code just to get some warnings actually caught is not worth it.

By the way, the warnings do show in the output of ./doctest, but, apparently, doctest misses them.

Thank you for telling me about gist :-) I'll use try it out next time.

@asmeurer
asmeurer Feb 22, 2012 Member

On Feb 20, 2012, at 2:19 PM, Sergiu Ivanov
reply@reply.github.com
wrote:

@@ -4,11 +4,82 @@
"""

class SymPyDeprecationWarning(DeprecationWarning):

  • def init(self, value):
  •    self.parameter = value
    
  • """A deprecation warning which can also hold the information about
  • the feature to be removed, when it will be removed, and what the
  • developers recommend to use instead.
  • This class is expected to be used with the warnings.warn function
  • (note that one has to explicitly turn on deprecation warnings):
  • from warnings import warn

  • from sympy.core.compatibility import SymPyDeprecationWarning

  • warnings.warn("Don't do this, it's deprecated", SymPyDeprecationWarning)

  • main:1: SymPyDeprecationWarning: "Don't do this, it's deprecated"

Thank you for your advise! I took the way of skipping the doctest altogether, instead of trying to get it pass. The reason is that, according to what is written here, one should either turn warnings on explicitly with simplefilter, which I did and it didn't work for me, or one should write a decorator. I chose to consider that writing so much code just to get some warnings actually caught is not worth it.

By the way, the warnings do show in the output of ./doctest, but, apparently, doctest misses them.

That might be because they are printed to stderr instead of stdout.

Thank you for telling me about gist :-) I'll use try it out next time.


Reply to this email directly or view it on GitHub:
https://github.com/sympy/sympy/pull/1065/files#r468703

@Krastanov Krastanov and 1 other commented on an outdated diff Feb 16, 2012
sympy/core/compatibility.py
+ __main__:3: SymPyDeprecationWarning: The feature such and such is
+ deprecated. Use this other feature instead. Contact the developers
+ for further information.
+
+ If, however, the argument value does not hold a string, a string
+ representation of the object will be appended to the message:
+
+ >>> warnings.warn(SymPyDeprecationWarning(feature = \"such and such\",
+ ... useinstead = \"this other feature\",
+ ... value = [1,2,3]))
+ __main__:3: SymPyDeprecationWarning: The feature such and such is
+ deprecated. Use this other feature instead. ([1, 2, 3])
+ """
+
+ def __init__(self, value = "", feature = "", expected_removal_version = "",
+ useinstead = ""):
@Krastanov
Krastanov Feb 16, 2012 Member

None seems like a better magic value (compared to "").

@scolobb
scolobb Feb 17, 2012 Contributor

Checking for "" would also take care of the situation when the user provides an empty string; do you think it's worth switching to None?

@Krastanov
Krastanov Feb 17, 2012 Member

Why would the user provide an empty string? But even in this case you can use if something:. The if block wont be executed for something == None / "" / [] / (,) / similar_empty_objects. Anyway, it's rather minor remark, don't worry about it.

@scolobb
scolobb Feb 20, 2012 Contributor

Thank you for pointing out these details. Now that you've said it, I can remember that I did read about it a while ago :-)

@Krastanov Krastanov commented on an outdated diff Feb 16, 2012
sympy/core/compatibility.py
+
+ If, however, the argument value does not hold a string, a string
+ representation of the object will be appended to the message:
+
+ >>> warnings.warn(SymPyDeprecationWarning(feature = \"such and such\",
+ ... useinstead = \"this other feature\",
+ ... value = [1,2,3]))
+ __main__:3: SymPyDeprecationWarning: The feature such and such is
+ deprecated. Use this other feature instead. ([1, 2, 3])
+ """
+
+ def __init__(self, value = "", feature = "", expected_removal_version = "",
+ useinstead = ""):
+ self.fullMessage = ""
+
+ if feature != "":
@Krastanov
Krastanov Feb 16, 2012 Member

You can simplify it to just:

if feature:
    blah
    blah
@asmeurer
Member

By the way, is sympy.core.compatibility really the right place for this? It seems to me that it really belongs somewhere in utilities.

@asmeurer
Member

I think it will be better to use last_supported_version instead of expected_removal_version. Or at least we should use both.

The issue arises when we do a major version release (0.8 for example).

@asmeurer asmeurer and 1 other commented on an outdated diff Feb 17, 2012
sympy/core/compatibility.py
+ if feature != "":
+ self.fullMessage = "The feature " + feature + " is deprecated."
+
+ if expected_removal_version != "":
+ self.fullMessage += " It is expected to be removed by SymPy version "
+ self.fullMessage += expected_removal_version + "."
+ if useinstead != "":
+ self.fullMessage += " Use " + useinstead + " instead."
+
+ if (self.fullMessage != ""):
+ # We should also handle a non-string "value".
+ if isinstance(value, str):
+ if value != "":
+ self.fullMessage += " " + value
+ else:
+ self.fullMessage += " (" + repr(value) + ")"
@asmeurer
asmeurer Feb 17, 2012 Member

Why do you use repr?

@scolobb
scolobb Feb 17, 2012 Contributor

I use repr because, originally, SymPyDeprecationWarning would use repr on its argument as well. I'm not sure I've seen this used anywhere across SymPy, but I decided to replicate the old behaviour anyway.

@asmeurer
Member

This would be a lot cleaner if you used (new style) string formatting.

@scolobb
Contributor
scolobb commented Feb 17, 2012

I found SymPyDeprecationWarning in compatibility, so I left it there. But indeed, having this warning in utilities would be cleaner, I think.

Do I move it into utilities, then?

@scolobb
Contributor
scolobb commented Feb 17, 2012

Now that I have made changes to the code, will it be okay if I drop the old commit, create a new one with the new version of the code and do a git push -f? Will it break this discussion page?

It's the first time that I'm using the pull request feature of GitHub, so sorry for dumb questions :-)

@flacjacket
Member

@scolobb That should be fine, the pull request goes by the branch name, not the commits themselves, so doing a force push is okay.

@scolobb
Contributor
scolobb commented Feb 20, 2012

I tried to comply with all suggestions given so far. I'm still not sure whether I should move the warning class out of sympy.core.compatibility, though.

@asmeurer
Member

We lose any comments on commits themselves when you do that, but that's just an issue that we have to deal with.

Go ahead and move it.

@scolobb
Contributor
scolobb commented Feb 21, 2012

Turns out moving SymPyDeprecationWarning under sympy.utilities generates circular imports: core imports utilities (because core needs SymPyDeprecationWarning) which imports core. Therefore, I only moved the warning class in a dedicated file under core, since it certainly has little to do with compatibility issues.

@scolobb
Contributor
scolobb commented Feb 21, 2012

I did expect that me forcefully pushing a commit would destroy the comments; thank you for bringing certainty :-)

@asmeurer
Member

You can fix circular imports by putting the import inside the
function, or sometimes by putting it at the bottom of the file. You
should be able to get it to work in utilities.

On Feb 21, 2012, at 3:34 PM, Sergiu Ivanov
reply@reply.github.com
wrote:

Turns out moving SymPyDeprecationWarning under sympy.utilities generates circular imports: core imports utilities (because core needs SymPyDeprecationWarning) which imports core. Therefore, I only moved the warning class in a dedicated file under core, since it certainly has little to do with compatibility issues.


Reply to this email directly or view it on GitHub:
#1065 (comment)

@asmeurer
Member

I always try to write comments on the diff because of this.

Note that it's not always necessary to squash everything: having your
history is often fine. It can be necessary to get rid of useless
commits sometimes, though, or to fix commit messages.

On Feb 21, 2012, at 3:35 PM, Sergiu Ivanov
reply@reply.github.com
wrote:

I did expect that me forcefully pushing a commit would destroy the comments; thank you for bringing certainty :-)


Reply to this email directly or view it on GitHub:
#1065 (comment)

@scolobb
Contributor
scolobb commented Feb 22, 2012

Indeed, I managed to get rid of circular references by moving a couple imports into the functions that used them. Thank you for the suggestion :-)

@scolobb
Contributor
scolobb commented Feb 22, 2012

I'm still a bit under the influence of the "puritan history" ideology. I don't take this to the extreme and I usually save the largest part of my history; I do invest some time into keeping the history clean, though :-)

@scolobb
Contributor
scolobb commented Feb 27, 2012

Sorry, I forgot to fix isympy in the previous commits :-( Should be fine now.

@scolobb
Contributor
scolobb commented Mar 3, 2012

SymPy Bot Summary: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY4IQODA

Interpreter: /usr/bin/python (2.7.2-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: c6542db
branch hash: a7476741b8d2eb2d4e2cc1aae92eeabaf57d56c9

Automatic review by SymPy Bot.

@scolobb
Contributor
scolobb commented Mar 3, 2012

SymPy Bot Summary: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYlZwODA

Interpreter: /usr/bin/python (2.7.2-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: c6542db
branch hash: 0971fa5ca373e0511c910e1028f2fe73a4a1e7e1

Automatic review by SymPy Bot.

@asmeurer
Member
asmeurer commented Mar 4, 2012

I'm still a bit under the influence of the "puritan history" ideology. I don't take this to the extreme and I usually save the largest part of my history; I do invest some time into keeping the history clean, though :-)

As long as you write good commit messages, this is actually the best way to go for the most part.

@asmeurer
Member
asmeurer commented Mar 4, 2012

Don't put any spaces around = when used with keyword arguments.

@asmeurer
Member
asmeurer commented Mar 4, 2012

Why not go ahead and change all deprecation warnings in SymPy to use these new features.

@asmeurer
Member
asmeurer commented Mar 4, 2012

SymPy Bot Summary: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYwOUNDA

Interpreter: /Library/Frameworks/Python.framework/Versions/3.2/bin/python3 (3.2.2-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: setup.py test
master hash: a5848d0
branch hash: 0971fa5ca373e0511c910e1028f2fe73a4a1e7e1

Automatic review by SymPy Bot.

@scolobb
Contributor
scolobb commented Mar 7, 2012

SymPy Bot Summary: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYnsMODA

Interpreter: /usr/bin/python (2.7.2-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: 5a2e159
branch hash: 3b0ea2bf14a2b84ac402aaf6b41fdc8c502c07fc

Automatic review by SymPy Bot.

@asmeurer asmeurer commented on an outdated diff Mar 7, 2012
sympy/core/symbol.py
@@ -61,10 +61,9 @@ def __new__(cls, name, commutative=True, **assumptions):
if 'dummy' in assumptions:
warnings.warn(
- "\nThe syntax Symbol('x', dummy=True) is deprecated and will"
- "\nbe dropped in a future version of SymPy. Please use Dummy()"
- "\nor symbols(..., cls=Dummy) to create dummy symbols.",
- SymPyDeprecationWarning)
+ SymPyDeprecationWarning(
+ "Deprecated syntax.", feature="Symbol('x', dummy=True)",
@asmeurer
asmeurer Mar 7, 2012 Member

The "Deprecated syntax." part here looks out of place. I would just remove it entirely.

@asmeurer
Member
asmeurer commented Mar 7, 2012

I get

In [1]: x.as_coeff_terms()
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
/Users/aaronmeurer/Documents/python/sympy/sympy/<ipython-input-1-5450fa42cf7b> in <module>()
----> 1 x.as_coeff_terms()

/Users/aaronmeurer/Documents/python/sympy/sympy/sympy/core/expr.py in as_coeff_terms(self, *deps)
   1426         """
   1427         import warnings
-> 1428         from exceptions import SymPyDeprecationWarning
   1429         warnings.warn(SymPyDeprecationWarning(feature="as_coeff_terms()",
   1430                                               useinstead="as_coeff_mul()"))

ImportError: cannot import name SymPyDeprecationWarning

In [2]: x.as_coeff_factors()
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
/Users/aaronmeurer/Documents/python/sympy/sympy/<ipython-input-2-8f90e52c3c20> in <module>()
----> 1 x.as_coeff_factors()

/Users/aaronmeurer/Documents/python/sympy/sympy/sympy/core/expr.py in as_coeff_factors(self, *deps)
   1436         """
   1437         import warnings
-> 1438         from exceptions import SymPyDeprecationWarning
   1439         warnings.warn(SymPyDeprecationWarning(feature="as_coeff_factors()",
   1440                                               useinstead="as_coeff_add()"))

ImportError: cannot import name SymPyDeprecationWarning
@asmeurer asmeurer commented on an outdated diff Mar 7, 2012
sympy/core/symbol.py
@@ -285,9 +284,10 @@ def symbols(names, **args):
"""
result = []
if 'each_char' in args:
- warnings.warn("The each_char option to symbols() and var() is "
- "deprecated. Separate symbol names by spaces or commas instead.",
- SymPyDeprecationWarning)
+ warnings.warn(
+ SymPyDeprecationWarning(
+ feature="each_char option to symbols() and var()",
@asmeurer
asmeurer Mar 7, 2012 Member

The way that this prints, "The feature each_char option to symbols() and var()...," seems grammatically incorrect, or at least grammatically sloppy. I think it would be similar in most cases to say, "The feature such and such is deprecated." I'm not sure what a good way to change it would be, though.

@asmeurer asmeurer commented on the diff Mar 7, 2012
sympy/utilities/exceptions.py
@@ -0,0 +1,79 @@
+"""
+General SymPy exceptions and warnings.
+"""
+
+class SymPyDeprecationWarning(DeprecationWarning):
+ r"""A warning for deprecated features of SymPy.
@asmeurer
asmeurer Mar 7, 2012 Member

Add a note about the decorator in this docstring.

@asmeurer
Member
asmeurer commented Mar 7, 2012

Well, sorry for misleading you, but apparently new-style string formatting does not work in Python 2.5. So we are going to have to find a different way to do it.

@asmeurer
Member
asmeurer commented Mar 7, 2012

Since you're just using really simple formatting, you can just use old style formatting. I actually wanted you to use the dictionary style, but I see from the structure of the code that this is unnecessary.

@asmeurer
Member
asmeurer commented Mar 7, 2012

This also brings up another point, which is that we need to actually test this somehow. Skipping the doctests is not an option. I think what we should do is note at the top how to use it (skipping this). For the formatting tests, you should define __str__ on SymPyDeprecationWarning and just create the object and show what message it would create. Like

>>> SymPyDeprecationWarning(feature="such and such",
    ...     useinstead="this other feature",
    ...     value=[1,2,3]))
SymPyDeprecationWarning("The feature such and such is deprecated.  Use this other feature instead.  ([1, 2, 3])")
@scolobb
Contributor
scolobb commented Mar 8, 2012

SymPy Bot Summary: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9fkODA

Interpreter: /usr/bin/python (2.7.2-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: 5135035
branch hash: 69f450324898494e90024ee53d0bff8241ecd265

Automatic review by SymPy Bot.

@scolobb
Contributor
scolobb commented Mar 8, 2012

I hope this is going to work with Python 2.5 (I don't really have the possibility to test with this Python version for now). This time I've added quite a number of minor commits; I can squash them a bit, if necessary.

@asmeurer
Member
asmeurer commented Mar 8, 2012

I can test 2.5, but due to a bug in SymPy-Bot, I can't upload them (see http://code.google.com/p/sympy/issues/detail?id=3036).

@asmeurer asmeurer commented on an outdated diff Mar 8, 2012
sympy/utilities/exceptions.py
+ self.fullMessage += " It will be last supported in SymPy version %s." % last_supported_version
+ if useinstead:
+ self.fullMessage += " Use %s instead." % useinstead
+
+ if self.fullMessage:
+ # We should also handle a non-string "value".
+ if isinstance(value, str):
+ if value:
+ self.fullMessage += " " + value
+ elif value:
+ self.fullMessage += " (%s)" % repr(value)
+ else:
+ # No extended arguments; replicate the original behaviour.
+ self.fullMessage = repr(value)
+
+ def __str__(self):
@asmeurer
asmeurer Mar 8, 2012 Member

You should define this like I said, as SymPyDeprecationWarning("message"), so that you can reproduce it from the string.

@scolobb
Contributor
scolobb commented Mar 9, 2012

This time I didn't rebase the branch before pushing it. GitHub renders this better, but does it make a difference for the maintainers?

@scolobb
Contributor
scolobb commented Mar 9, 2012

SymPy Bot Summary: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYxokPDA

Interpreter: /usr/bin/python (2.7.2-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: f102465
branch hash: 4288d2a30adbbe32c7bf9eb87adae1655adee153

Automatic review by SymPy Bot.

@asmeurer
Member

Sorry for not getting back. I've been backlogged with all the GSoC traffic.

You need to rebase/merge this again. It doesn't really matter which one you do. We usually recommend rebase to new commiters, because they usually have to edit their commit messages and squash commits, but you are already doing a decent job of that anyway.

scolobb added some commits Feb 16, 2012
@scolobb scolobb Extend SymPyDeprecationWarning with additional information. (Issue 2142)
SymPyDeprecationWarning now offers convenient facilities for
specifying the deprecated feature, the version in which the feature
is expected to be removed and what the user should use instead of
the deprecated feature.
86ca95b
@scolobb scolobb Move SymPyDeprecationWarning into sympy.core.exceptions. 72386a1
@scolobb
Contributor
scolobb commented Mar 24, 2012

SymPy Bot Summary: There were test failures.

@scolobb: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYzYMRDA

Interpreter: /usr/bin/python (2.7.2-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: 22951cf
branch hash: 3b0de7aff5d7efc2b6f147d19b911eaedcdff1d6

Automatic review by SymPy Bot.

@scolobb
Contributor
scolobb commented Mar 24, 2012

SymPy Bot Summary: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY_PMQDA

Interpreter: /usr/bin/python (2.7.2-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: 22951cf
branch hash: 327469c3522580c9925d6bc9f596f3b9fa4bd789

Automatic review by SymPy Bot.

@scolobb
Contributor
scolobb commented Mar 24, 2012

No problem, I can easily see that GSoC poses a significant burden on the maintainers of the project.

I have rebased the branch, because, according to my understanding, rebasing is how changes get from master in a topic branch, while merging is how updates come from a topic branch into master. I do understand that either of the operations can be theoretically used to do both; I just like rebase better :-)

Sorry about that extra test failure -- I often forget to clear the left-over .pyc files :-(

@asmeurer
Member

No problem, I can easily see that GSoC poses a significant burden on the maintainers of the project.

Yes, it does, but don't worry, things will cool down again once the application period is over. Unfortunately, we need to review pull requests before then :).

I have rebased the branch, because, according to my understanding, rebasing is how changes get from master in a topic branch, while merging is how updates come from a topic branch into master. I do understand that either of the operations can be theoretically used to do both; I just like rebase better :-)

That's not exactly a correct interpretation. The difference is what it does to the commits. It's easier to visualize with something like gitk. Rebase takes all the commits and reapplies them one at a time on top of master, so that you have a linear history. Merging creates a special commit, called a merge commit, that linkes the two branches.

To understand the difference, you should understand how commits work. A commit is just a changeset that is applied on top of other changesets. Each commit is hashed using the SHA1 algorithm. The hash includes the changes of the commit, the commit message, some metadata like the date and author, and the SHA1 hash(s) of the commit(s) it was applied on top of. Because the hash includes the previous commit's hash in it, a commit actually contains information about its entire history.

Now, because rebase takes commits and reapplies them, this effectively creates new commits. The SHA1 has will change, and as far as git is concerned, they are different. That's why when you rebased, it moved the commits later in pull request discussion. For example, 3b0ea2bf14a2b84ac402aaf6b41fdc8c502c07fc is now c19cce1912bcc8fd48b45a21c37e210ad251e11b.

When you merge, on the other hand, the commits are not changed at all. Every commit remains exactly the same. Rather, a new commit is made that links the two branches together.

The advantage of merging is that if someone else has work based off of your history, it will not affect them. If you rebase, since it changes the history, it will mess things up, and you can end up with duplicate commits in the history. Another advantage to merging is that you can see what the merge conflicts were and how they were resolved, because that information will be in the merge commit. If you rebase, you lose this information, because only the new commits will remain.

The advantage of rebasing is that you can go ahead and modify other aspects of the history as well, such as editing commit messages, squashing or splitting commits, deleting commits, injecting new commits, or any other way you can think of. All of these would change the commits's SHA1, so they will create new commits. Rebasing also creates a nice linear history, which looks nicer in gitk (but there's really no other reason why this is desirable).

Since no one else was using your code, merging or rebasing didn't matter. If in the future you are working on a large branch, and other people may be working off of it, you should always merge, or else it will mess them up.

Is that all clear?

Sorry about that extra test failure -- I often forget to clear the left-over .pyc files :-(

That shouldn't be an issue, especially with sympy-bot. Are you sure that's what happened?

@asmeurer
Member

TODO: verify that running deprecated behavior produces the correct warning.

@asmeurer
Member

You still didn't fix this:


In [1]: x.as_coeff_terms()
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
/Users/aaronmeurer/Documents/Python/sympy/sympy/<ipython-input-1-5450fa42cf7b> in <module>()
----> 1 x.as_coeff_terms()

/Users/aaronmeurer/Documents/Python/sympy/sympy/sympy/core/expr.py in as_coeff_terms(self, *deps)
   1491         """
   1492         import warnings
-> 1493         from exceptions import SymPyDeprecationWarning
   1494         warnings.warn(SymPyDeprecationWarning(feature="as_coeff_terms()",
   1495                                               useinstead="as_coeff_mul()"))

ImportError: cannot import name SymPyDeprecationWarning

In [2]: x.as_coeff_factors()
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
/Users/aaronmeurer/Documents/Python/sympy/sympy/<ipython-input-2-8f90e52c3c20> in <module>()
----> 1 x.as_coeff_factors()

/Users/aaronmeurer/Documents/Python/sympy/sympy/sympy/core/expr.py in as_coeff_factors(self, *deps)
   1501         """
   1502         import warnings
-> 1503         from exceptions import SymPyDeprecationWarning
   1504         warnings.warn(SymPyDeprecationWarning(feature="as_coeff_factors()",
   1505                                               useinstead="as_coeff_add()"))

ImportError: cannot import name SymPyDeprecationWarning
@scolobb
Contributor
scolobb commented Mar 24, 2012

Oh wow :-( Sorry for that :-( I will check all imports and will then try to trigger all deprecation warnings to check if they are thrown properly.

It's late night for me, so I'll get back to work in about 8 hours.

@asmeurer
Member

SymPy Bot Summary: There were test failures.

@scolobb: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYtosRDA

Interpreter: /usr/local/bin/python2.5 (2.5.0-final-0)
Architecture: Darwin (32-bit)
Cache: yes
Test command: setup.py test
master hash: 22951cf
branch hash: 327469c3522580c9925d6bc9f596f3b9fa4bd789

Automatic review by SymPy Bot.

@asmeurer
Member

SymPy Bot Summary: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY5vsQDA

Interpreter: /Library/Frameworks/Python.framework/Versions/3.2/bin/python3 (3.2.2-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: setup.py test
master hash: 22951cf
branch hash: 327469c3522580c9925d6bc9f596f3b9fa4bd789

Automatic review by SymPy Bot.

@scolobb
Contributor
scolobb commented Mar 25, 2012

SymPy Bot Summary: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYgPQQDA

Interpreter: /usr/bin/python (2.7.2-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: 3400a17
branch hash: e7af273

Automatic review by SymPy Bot.

@scolobb
Contributor
scolobb commented Mar 25, 2012

Since no one else was using your code, merging or rebasing didn't matter. If in the future you are working on a large branch, and other people may be working off of it, you should always merge, or else it will mess them up.

Is that all clear?

Yes, absolutely. I have never managed a repository on which more than one person worked, and I've got very used to rebase and to fixing commits every now and then. I did of course remark that the commits changed after the rebase and I realised that it would affect other developers who would work off my branch.

I cannot say that the information your provided was all new to me, however, it brought together different things which lay asunder in my head and now I have a much better perspective. Thank you very much, I do appreciate the time you have invested into writing this extensive explanation!

Since nobody is working off my deprecation-warning branch, I'll stick to rebasing. However, I will carefully consider merging as well in the future. Thank you again :-)

Sorry about that extra test failure -- I often forget to clear the left-over .pyc files :-(

That shouldn't be an issue, especially with sympy-bot. Are you sure that's what happened?

What happened is that, while solving a merge conflict during rebasing I accidentally removed a necessary import. However, since that file had already been compiled to a .pyc, the tests passed fine. When I pushed my changes, and try to review the pull request with sympy-bot, I got the test failures I referred to.

TODO: verify that running deprecated behavior produces the correct warning.

Done; this resulted in a new minor commit: e7af273 :-)

@scolobb
Contributor
scolobb commented Mar 25, 2012

I'm not sure how to fix the failures with Python 2.5. The test report says:

Detected SAGE64 flag
Building Sage on OS X in 64-bit mode
�[?1034h/private/var/folders/wc/dppcpmxs1tlb36nqcw853wkm0000gn/T/sympy-bot-tmpMpugRf/sympy/sympy/external/importtools.py:152: UserWarning: gmpy version is too old to use (1.03 or newer required)
UserWarning)
init2.c:52: MPFR assertion failed: p >= 2 && p <= ((mpfr_prec_t)((mpfr_prec_t)(~(mpfr_prec_t)0)>>1))


Unhandled SIGABRT: An abort() occurred in Sage.
This probably occurred because a compiled component of Sage has a bug
in it and is not properly wrapped with sig_on(), sig_off(). You might
want to run Sage under gdb with 'sage -gdb' to debug this.
Sage will now terminate.


/Applications/sage/local/bin/sage-sage: line 449: 52419 Abort trap: 6 python "$@"

I'm not sure this is in any way related to the changes introduced in my pull request.

@scolobb
Contributor
scolobb commented Mar 25, 2012

The test failure in #1108 (comment) is exactly the same.

@asmeurer
Member

By the way, there are good reasons to not rebase even if you are working alone. One of the reasons for using version control is to keep track of your changes so that you can backtrack and find where something broke (e.g., using git bisect). If you rebase, the commit history is no longer the true commit history. Very often, rebasing causes old commits to break. Other times, you can "rebase out" the change from a commit, meaning that the change it represents is no longer the one described in the commit message.

@asmeurer
Member

Sorry for taking so long to finish reviewing this. This can go on in now.

@asmeurer asmeurer merged commit bba53f5 into sympy:master Apr 13, 2012
@scolobb
Contributor
scolobb commented Apr 13, 2012

Sorry for taking so long to finish reviewing this. This can go on in now.

No problem. Glad to see it merged.

By the way, there are good reasons to not rebase even if you are working alone.

That sounds right. I've never used git merge (which slightly surprises me now when I realise it). Looks like it's going to be an interesting and useful experience to try it. Thank you for the information!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment