Opened 20 months ago

Closed 19 months ago

Last modified 18 months ago

#19979 closed enhancement (fixed)

Improving Coding Style and Documentation in projective product schemes

Reported by: rlmiller Owned by: rlmiller
Priority: trivial Milestone: sage-7.1
Component: algebraic geometry Keywords:
Cc: Merged in:
Authors: Rebecca Lauren Miller Reviewers: Ben Hutz
Report Upstream: N/A Work issues:
Branch: 7b099f7 (Commits) Commit:
Dependencies: Stopgaps:

Description

There are number of coding style issues and documentation issues in the affine scheme functionality such as

  • == False or == True
  • references to self in docs
  • no single line description
  • formatting of errors

Change History (21)

comment:1 Changed 20 months ago by rlmiller

  • Branch set to u/rlmiller/test

comment:2 Changed 20 months ago by rlmiller

  • Commit set to 8264cabe9eb30680f80026c10223e56108e4bc99
  • Summary changed from Improving Codine Style and Documentation in projective schemes to Improving Coding Style and Documentation in projective product schemes

Last 10 new commits:

25e1132Trac 17785: fix segmentation fault for subs
bf8ee82#19880 update openssl to 1.0.2e
ccf6ad4Updated Sage version to 7.0.rc1
a8b26feDeprecate composite_field()
0214b33Move sage.rings.arith to sage.arith.misc
bcd965dImprove location change error message
d41d5cbRaise an error if Sage has been relocated
b5ed4aaUpdated Sage version to 7.0
ec0093e19979 Fixing in style in projective products
8264cab19979 Fixing style of Product Projective folder

comment:3 Changed 20 months ago by rlmiller

  • Branch changed from u/rlmiller/test to u/rlmiller/t/19979
  • Commit changed from 8264cabe9eb30680f80026c10223e56108e4bc99 to 10f66b453e67b3367aff3a9537e84c4f3e5f37bc

Something went wrong with the rebase. Here is a corrected branch.


New commits:

10f66b419979 Improve coding for product projectives

comment:4 Changed 20 months ago by git

  • Commit changed from 10f66b453e67b3367aff3a9537e84c4f3e5f37bc to a5b09747394eb66468d13eb788d32805b3da122f

Branch pushed to git repo; I updated commit sha1. New commits:

a5b0974Merge branch 'master' into t/19979/t/19979

comment:5 Changed 20 months ago by rlmiller

  • Status changed from new to needs_review

Something went wrong with the rebase. Here is a corrected branch.


New commits:

10f66b419979 Improve coding for product projectives
a5b0974Merge branch 'master' into t/19979/t/19979

New commits:

10f66b419979 Improve coding for product projectives
a5b0974Merge branch 'master' into t/19979/t/19979

comment:6 Changed 20 months ago by rlmiller

  • Cc bhutz added

comment:7 Changed 20 months ago by bhutz

  • Status changed from needs_review to needs_work

merge conflict with the latest beta.

comment:8 Changed 20 months ago by git

  • Commit changed from a5b09747394eb66468d13eb788d32805b3da122f to 7ddbc55b5e88bedb515f9b3db60752ad61e2e099

Branch pushed to git repo; I updated commit sha1. New commits:

1803633Merge branch 'master' into t/19979/t/19979
7ddbc5519979 Updating style of Product Projective folder

comment:9 Changed 20 months ago by rlmiller

  • Status changed from needs_work to needs_review

comment:10 Changed 20 months ago by bhutz

  • Cc bhutz removed
  • Reviewers set to Ben Hutz
  • Status changed from needs_review to needs_work

The docs don't build:

[schemes  ] /home/ben/sage/sage-test/local/lib/python2.7/site-
packages/sage/schemes/product_projective/space.py:docstring of 
sage.schemes.product_projective.space.is_ProductProjectiveSpaces:1: WARNING: Inline interpreted text or phrase reference start-string without end-string.

wehlerK3: I started looking at this line by line, but there are many repeating issues so I'll look at this file line-by-line after you make another pass.

  • no spaces after ( or before )
  • bilinear is spelled wrong many times

here are the first few instances of these:

  • import spacing
  • line 72 bilinear misspelled
  • line 75: OUTPUT: :class:WehlerK3Surface_ring
  • line 102: no space for (space
  • 106: bilinear
  • 110 OUTPUT: :class:WehlerK3Surface_ring
  • 118 wrap line
  • 129: no spaces before/after (, )
  • 135: bilinear
  • 150, 159, 161, 163, 166, 168, 170, 173: paren space
  • 182 output
  • 199 paren space, no need for () around return
  • 207 AttributeError?, True if point is on surface
  • 243 paren space
  • 245 need better one line description
  • 249, 251, 261, 285, 287 paren space
  • 291 better one line description

Here is what I saw in the other files:

space.py:

  • is_ProduceProjectiveSpaces: shorter one line description
  • 137: spaces
  • 428 one line
  • 443 ending punc
  • 447 .
  • 549 onthis
  • 733 R

point.py

  • 82: spaces
  • 99,103,107 i
  • 237 thid
  • 294 t
  • 333: self
  • 351: parameter spaces
  • 373: , QQ
  • 391 spaces
  • 415 n -> N, P is self, so this doc string needs to be reworded
  • 417 n -> N, self/P are not right
  • 424 self
  • 457 spaces
  • 460 codomain
  • 461 N, space (

morphism.py

  • 31 blank line before
  • 78, 79, 90, 91: , QQ)
  • 124,128,132 i
  • 188 , [1,1,1]
  • 226 space
  • 240 iff -> if and only if
  • 323 thus -> this, n -> N
  • 359: just start this with "This function..."
  • 364 reducing -> reduce
  • 389 codomain

comment:11 Changed 19 months ago by git

  • Commit changed from 7ddbc55b5e88bedb515f9b3db60752ad61e2e099 to 5fe2f41978ac77f4cd45f9494b148f2b54daa76f

Branch pushed to git repo; I updated commit sha1. New commits:

5fe2f4119979 Fixing style of product projective folder

comment:12 Changed 19 months ago by rlmiller

  • Status changed from needs_work to needs_review

comment:13 Changed 19 months ago by bhutz

  • Branch changed from u/rlmiller/t/19979 to u/bhutz/t/19979
  • Commit changed from 5fe2f41978ac77f4cd45f9494b148f2b54daa76f to 7b099f76e9e6217c03320d0c9d85f8011a1b2202

I made some small additional corrections. You should double check those and see if we've missed anything else.


New commits:

7b099f719979: some small additional changes

comment:14 Changed 19 months ago by rlmiller

Changes look fine.

comment:15 Changed 19 months ago by bhutz

  • Status changed from needs_review to positive_review

ok. we should be all set here then.

comment:16 Changed 19 months ago by vbraun

  • Status changed from positive_review to needs_work

Author name...

comment:17 Changed 19 months ago by bhutz

  • Authors set to Rebecca Miller
  • Status changed from needs_work to positive_review

comment:18 Changed 19 months ago by vbraun

  • Branch changed from u/bhutz/t/19979 to 7b099f76e9e6217c03320d0c9d85f8011a1b2202
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:19 Changed 18 months ago by jdemeyer

  • Commit 7b099f76e9e6217c03320d0c9d85f8011a1b2202 deleted

What do you prefer: "Rebecca Lauren Miller" or "Rebecca Miller"?

comment:20 Changed 18 months ago by kcrisman

Given that comment:6:ticket:20067 was done by rlmiller while this one was by bhutz, I would guess the "Lauren" version.

comment:21 Changed 18 months ago by rlmiller

  • Authors changed from Rebecca Miller to Rebecca Lauren Miller
Note: See TracTickets for help on using tickets.