Closed tscrim closed 7 years ago
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c028abb | Adding QQ to NumberFields. |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
32a4e16 | Adding QQ to NumberFields. |
Hi Travis, I was just busy reviewing this ticket and then I saw a new commit. I noticed that you like to force push new stuff to trac, but I strongly dislike this. Once you have made your changes public you should not force push, because
1) it screws up other people who have pulled your branch, and made changes on top of it.
2) It destroys history, for example it is now harder to review this ticket for me. If you didn't force push, I could just could have looked at the diff of your most recent commit and your second most recent commit and I could see if the extra additions make sense. Now however I need to start from scratch.
Here is a good read by Linus Torvaldus on how one should use git and be nice to others: http://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html
Luckily this is a small ticket so I am not to annoyed, but still a little bit annoyed non the less.
New commits:
32a4e16 | Adding QQ to NumberFields. |
In my original review I was going to say that it looks good to me except that you missed 4 files where there are also categories printed that depend on the category of QQ. But save that, that it looks good to me.
If you didn't force push you would have saved me the trouble to manually compare if your latest commit did noting except changing those 4 doctests.
P.s. I also tested if this happens to give QQ any new methods, but sadly enough there are no new ones that show up with tab completion. I still think it is a good change though.
Thank you for the review. If you could add in your name to the reviewer field.
Thank you also for the lecture. It's not like this ticket was sitting here for that long, so I figured it was safe to do so to keep a clean history. Additionally, from having dealt with merging in rebased branches, git is pretty good with merging equal changes to setup the diff.
Sorry, that was very passive-aggressive of me. I didn't think anyone would be looking at this ticket in the timespan from when I first posted it, and I apologize for the trouble.
Reviewer: Maarten Derickx
Changed branch from public/number_fields/QQ_number_field-23761 to 32a4e16
Because QQ is a number field, but Sage doesn't recognize it as one:
Related to #23245 and #7596.
CC: @fchapoton @jdemeyer @videlec @koffie
Component: number fields
Author: Travis Scrimshaw
Branch:
32a4e16
Reviewer: Maarten Derickx
Issue created by migration from https://trac.sagemath.org/ticket/23761