Opened 4 years ago
Closed 4 years ago
#23761 closed enhancement (fixed)
Add QQ to the category of number fields
Reported by:  tscrim  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  number fields  Keywords:  
Cc:  chapoton, jdemeyer, delecroix, mderickx  Merged in:  
Authors:  Travis Scrimshaw  Reviewers:  Maarten Derickx 
Report Upstream:  N/A  Work issues:  
Branch:  32a4e16 (Commits, GitHub, GitLab)  Commit:  32a4e1655ab6ed74bc90f28b86187fa6918a348b 
Dependencies:  Stopgaps: 
Description
Because QQ is a number field, but Sage doesn't recognize it as one:
sage: QQ.categories() [Join of Category of quotient fields and Category of metric spaces, Category of quotient fields, Category of fields, Category of euclidean domains, Category of division rings, Category of principal ideal domains, Category of unique factorization domains, Category of gcd domains, Category of integral domains, Category of domains, Category of commutative rings, Category of rings, Category of rngs, Category of semirings, Category of associative additive commutative additive associative additive unital distributive magmas and additive magmas, Category of additive commutative additive associative additive unital distributive magmas and additive magmas, Category of additive commutative additive associative distributive magmas and additive magmas, Category of additive associative distributive magmas and additive magmas, Category of distributive magmas and additive magmas, Category of magmas and additive magmas, Category of commutative monoids, Category of monoids, Category of semigroups, Category of commutative magmas, Category of unital magmas, Category of magmas, Category of commutative additive groups, Category of additive groups, Category of additive inverse additive unital additive magmas, Category of commutative additive monoids, Category of additive monoids, Category of additive unital additive magmas, Category of commutative additive semigroups, Category of additive commutative additive magmas, Category of additive semigroups, Category of additive magmas, Category of metric spaces, Category of topological spaces, Category of sets, Category of sets with partial maps, Category of objects]
Change History (10)
comment:1 Changed 4 years ago by
 Branch set to public/number_fields/QQ_number_field23761
 Commit set to 20d8949ddd671d34067dc7cdd61d24aa580572fa
 Status changed from new to needs_review
comment:2 Changed 4 years ago by
 Commit changed from 20d8949ddd671d34067dc7cdd61d24aa580572fa to c028abb2b6f968d5ab37fd22fd895cb7c80de34f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c028abb  Adding QQ to NumberFields.

comment:3 Changed 4 years ago by
 Commit changed from c028abb2b6f968d5ab37fd22fd895cb7c80de34f to 32a4e1655ab6ed74bc90f28b86187fa6918a348b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
32a4e16  Adding QQ to NumberFields.

comment:4 Changed 4 years ago by
 Status changed from needs_review to needs_work
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.mailarchive.com/dridevel@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.

comment:5 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:6 Changed 4 years ago by
 Status changed from needs_review to positive_review
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.
comment:7 Changed 4 years ago by
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.
comment:8 Changed 4 years ago by
Sorry, that was very passiveaggressive 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.
comment:9 Changed 4 years ago by
 Reviewers set to Maarten Derickx
comment:10 Changed 4 years ago by
 Branch changed from public/number_fields/QQ_number_field23761 to 32a4e1655ab6ed74bc90f28b86187fa6918a348b
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Adding QQ to NumberFields.