<< Back to previous view

[CLJ-2049] Improve clojure.zip documentation Created: 25/Oct/16  Updated: 27/Oct/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8, Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Brighid M Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, zip
Environment:

All


Attachments: File improve-zip-docs.diff    
Patch: Code
Approval: Triaged

 Description   

The clojure.zip module has extremely terse docstrings that are helpful as reference material but completely unhelpful to someone approaching the module cold. Expanded docstrings would make this piece of code much more visible to users who might find it helpful.



 Comments   
Comment by Brighid M [ 25/Oct/16 9:03 PM ]

A patch that improves clojure.zip's docstrings.

Comment by Alex Miller [ 27/Oct/16 8:42 AM ]

I would prefer if we could minimize the size of the changes by removing diffs that just add periods, change line breaks, add whitespace, or make other non-essential changes. That will help focus on the things that matter like changes in the ns docstring, zipper, etc. Additionally this ticket talks about doc strings but also makes changes in exception messages - I'd prefer code changes to be in a separate ticket. Also, please don't remove the comment sections in the files.

If you would like to convert the test comment section into actual tests, that would be a great separate ticket (I feel like I might have even written a patch to do that at one point but I don't see a ticket for it!).

Keeping this patch entirely focused on essential docstring changes is the best way to ensure its timely inclusion.

Comment by Brighid M [ 27/Oct/16 4:32 PM ]

Alex — to clarify, I'm hearing "this ticket should include two patches: one with the major prose additions and one with small proofreading-ish changes" and "this ticket's patches should not include the changes to exception messages nor the comment-move"?

Supplemental question: is there a style guide hanging around for the code & documentation style of the Clojure core? I poked around for such a guide on clojure.org and in JIRA: I didn't find one, but maybe I overlooked it. I was looking for a guideline about the comment section. It would be good to have a hint about "please don't touch these," because AFAICT they don't communicate that by themselves.

Comment by Alex Miller [ 27/Oct/16 5:43 PM ]

Actually, I'd prefer to have just one patch with the major prose additions. I don't think the minor changes are worth doing and will be a distraction from the more important prose.

Unfortunately, there is no style guide for Clojure core, and in fact it's been written by many people over many years so there generally isn't a consistent style throughout the code. Generally Rich prefers that debug code or comments that trace to him are left intact (git blame can help there). More generally, the simpler a patch is to review, the easier it is for it to stay good and be easily reviewed.

Thanks!

Comment by Brighid M [ 27/Oct/16 6:34 PM ]

> Actually, I'd prefer to have just one patch with the major prose additions.
Okay, I'll produce that patch and attach it to this ticket.

> I don't think the minor changes are worth doing and will be a distraction from the more important prose. Unfortunately, there is no style guide for Clojure core, and in fact it's been written by many people over many years so there generally isn't a consistent style throughout the code.
I think you just made an argument for why it is worth doing minor changes: consistency generally makes things more accessible. However, now is clearly not the time to litigate that, so I'm gonna come back with just the ns/docstring version of this patch.





[CLJ-1317] clojure.zip/seq-zip returns spurious nils during traversal Created: 31/Dec/13  Updated: 10/Feb/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Unresolved Votes: 1
Labels: zip

Attachments: Text File 0001-CLJ-1317-fix-seq-zip-to-avoid-spurious-nils.patch    
Patch: Code
Approval: Triaged

 Description   

Problem reported by Lee Spector on the mailing list:

https://groups.google.com/d/msg/clojure/8TL7IGmE7N0/u1xfgTOLDRgJ

Here's a quote from Lee's post describing the problem:

Here's an illustration, stepping through '(() 0) with next and printing the node at each step: 

(loop [z (zip/seq-zip '(() 0))] 
  (if (zip/end? z) 
    :done 
    (do (println (zip/node z)) 
      (recur (zip/next z))))) 

That produces: 

(() 0) 
() 
nil 
0 
:done 

I don't expect the nil to be there. 

The underlying cause is that seq-zip passes identity as the children argument to zipper. Applied to (), this returns (), which is truthy, leading zipper to descend into a non-existent subtree.

One natural solution would be to use seq in place of identity:

(defn seq-zip [root]
  (zipper seq?
          seq  ;; changed
          (fn [node children] (with-meta children (meta node)))
          root))

With this change, no nil is produced in the example above. Patch with this change forthcoming.



 Comments   
Comment by Michał Marczyk [ 31/Dec/13 5:52 PM ]

Note that the docstring of clojure.zip/zipper asks that the children argument return a seq of children. The rest of clojure.zip, however, expects nil to be returned when there are no children, as evidenced by this problem.

One could argue that this behaviour of the rest of clojure.zip should be fixed, but I think it makes sense and is convenient. Perhaps the docstring should be adjusted, though.

Comment by Alex Miller [ 08/Feb/16 4:36 PM ]

Michał, can I ask why you assigned this to yourself - was there something you planned to add?

Comment by Michał Marczyk [ 10/Feb/16 9:09 AM ]

Hey Alex, I was going to attach a separate patch with a proposal for a docstring adjustment along the lines suggested above (will do that tonight). No change to the code, though, and I guess not worth assigning the ticket – sorry about the unnecessary ping.

Comment by Alex Miller [ 10/Feb/16 9:38 AM ]

No worries, just wanted to know if something was still pending - I will wait to prescreen it.





[CLJ-1097] node-seq for clojure.zip Created: 29/Oct/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: zip

Attachments: File node-seq.diff    
Patch: Code and Test

 Description   

Many times it's easier to get to a zipper node via (first (filter pred ...)) instead of manually walking the tree via next/up/down. Other times it's easier to process certain nodes via filter & map instead of again, walking the tree. This patch provides a single function called node-seq that uses zip/next to generate a lazy-seq of nodes. Tests provide two examples.






[CLJ-941] NullPointerException possible with seq-zip Created: 26/Feb/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: zip


 Description   

For example:

Clojure 1.3.0
user=> (require '[clojure.zip :as z])
nil
user=> (-> (z/seq-zip (list 1)) z/down z/remove)
NullPointerException clojure.core/with-meta (core.clj:211)

Possibly the make-node function for seq-zip should be:

(fn [node children] (with-meta (or children ()) (meta node)))



 Comments   
Comment by Greg Chapman [ 26/Feb/12 5:54 PM ]

Also the docstring for zipper should probably be updated to indicate that the children parameter can be nil.





Generated at Sat Dec 10 06:54:30 CST 2016 using JIRA 4.4#649-r158309.