<< Back to previous view

[CLJ-1673] Improve clojure.repl/dir-fn to work on namespace aliases in addition to canonical namespaces. Created: 11/Mar/15  Updated: 10/Nov/15

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

Type: Enhancement Priority: Minor
Reporter: Jason Whitlark Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft, repl

Attachments: Text File clj-1673-2.patch     Text File clj-1673-3.patch     Text File improve_dir.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Extend clojure.repl/dir to work with the aliases in the current namespace

After:

user=> (require '[clojure.string :as s])
nil
user=> (dir s)
blank?
capitalize
...etc

Patch: clj-1673-3.patch



 Comments   
Comment by Jason Whitlark [ 11/Mar/15 4:00 PM ]

Possible unit test, since clojure.string is aliased in the test file:

(is (= (dir-fn 'clojure.string) (dir-fn 'str)))

Comment by Alex Miller [ 29/Apr/15 2:12 PM ]

Please add a test...

Comment by Jason Whitlark [ 02/May/15 10:35 PM ]

Updated patch, tweaked dir-fn, added test.

Comment by Jason Whitlark [ 02/May/15 10:38 PM ]

Should be good to go. I removed the old patch.

Comment by Alex Miller [ 04/May/15 4:38 PM ]

Tweaked patch just to remove errant blank line, otherwise same.

Comment by Michael Blume [ 12/Oct/15 5:54 PM ]

Applied this patch directly to master, test failure:

[java] ERROR in (test-dir) (core.clj:4023)
     [java] expected: (= (dir-fn (quote clojure.string)) (dir-fn (quote str)))
     [java]   actual: java.lang.Exception: No namespace: str found
     [java]  at clojure.core$the_ns.invokeStatic (core.clj:4023)
     [java]     clojure.repl$dir_fn.invokeStatic (repl.clj:186)
     [java]     clojure.repl$dir_fn.invoke (repl.clj:-1)
     [java]     clojure.test_clojure.repl$fn__21967$fn__21976.invoke (repl.clj:28)
     [java]     clojure.core$with_redefs_fn.invokeStatic (core.clj:7211)
     [java]     clojure.core$with_redefs_fn.invoke (core.clj:-1)
     [java]     clojure.test_clojure.repl$fn__21967.invokeStatic (repl.clj:27)
     [java]     clojure.test_clojure.repl/fn (repl.clj:-1)
     [java]     clojure.test$test_var$fn__7962.invoke (test.clj:703)
     [java]     clojure.test$test_var.invokeStatic (test.clj:703)
     [java]     clojure.test$test_var.invoke (test.clj:-1)
     [java]     clojure.test$test_vars$fn__7984$fn__7989.invoke (test.clj:721)
     [java]     clojure.test$default_fixture.invokeStatic (test.clj:673)
     [java]     clojure.test$default_fixture.invoke (test.clj:-1)
     [java]     clojure.test$test_vars$fn__7984.invoke (test.clj:721)
     [java]     clojure.test$default_fixture.invokeStatic (test.clj:673)
     [java]     clojure.test$default_fixture.invoke (test.clj:-1)
     [java]     clojure.test$test_vars.invokeStatic (test.clj:717)
     [java]     clojure.test$test_all_vars.invokeStatic (test.clj:723)
     [java]     clojure.test$test_ns.invokeStatic (test.clj:744)
     [java]     clojure.test$test_ns.invoke (test.clj:-1)
     [java]     clojure.core$map$fn__4770.invoke (core.clj:2639)
     [java]     clojure.lang.LazySeq.sval (LazySeq.java:40)
     [java]     clojure.lang.LazySeq.seq (LazySeq.java:49)
     [java]     clojure.lang.Cons.next (Cons.java:39)
     [java]     clojure.lang.RT.next (RT.java:688)
     [java]     clojure.core$next__4327.invokeStatic (core.clj:64)
     [java]     clojure.core$reduce1.invokeStatic (core.clj:924)
     [java]     clojure.core$reduce1.invokeStatic (core.clj:914)
     [java]     clojure.core$merge_with.invokeStatic (core.clj:2943)
     [java]     clojure.core$merge_with.doInvoke (core.clj:-1)
     [java]     clojure.lang.RestFn.applyTo (RestFn.java:139)
     [java]     clojure.core$apply.invokeStatic (core.clj:647)
     [java]     clojure.test$run_tests.invokeStatic (test.clj:754)
     [java]     clojure.test$run_tests.doInvoke (test.clj:-1)
     [java]     clojure.lang.RestFn.applyTo (RestFn.java:137)
     [java]     clojure.core$apply.invokeStatic (core.clj:645)
     [java]     clojure.core$apply.invoke (core.clj:-1)
     [java]     user$eval29133.invokeStatic (run_test.clj:8)
     [java]     user$eval29133.invoke (run_test.clj:-1)
     [java]     clojure.lang.Compiler.eval (Compiler.java:6934)
     [java]     clojure.lang.Compiler.load (Compiler.java:7381)
     [java]     clojure.lang.Compiler.loadFile (Compiler.java:7319)
     [java]     clojure.main$load_script.invokeStatic (main.clj:275)
     [java]     clojure.main$script_opt.invokeStatic (main.clj:335)
     [java]     clojure.main$script_opt.invoke (main.clj:-1)
     [java]     clojure.main$main.invokeStatic (main.clj:421)
     [java]     clojure.main$main.doInvoke (main.clj:-1)
     [java]     clojure.lang.RestFn.invoke (RestFn.java:408)
     [java]     clojure.lang.Var.invoke (Var.java:379)
     [java]     clojure.lang.AFn.applyToHelper (AFn.java:154)
     [java]     clojure.lang.Var.applyTo (Var.java:700)
     [java]     clojure.main.main (main.java:37)
Comment by Jason Whitlark [ 12/Oct/15 7:22 PM ]

After poking at it for a while, I discovered that the test in question isn't being executed in clojure.test-clojure.repl namespace, but in user. So replacing the failing test with:
(is (= (the-ns 'clojure.test-clojure.repl) ns))
also fails.

Which is very surprising to me. Is this the expected behavior? I could fix the test by doing the following:

(binding [*ns* (the-ns 'clojure.test-clojure.repl)]
(is (= (dir-fn 'clojure.string) (dir-fn 'str))))

But I don't want to mask an error, if this behavior is unexpected. Guidance please.

Comment by Alex Miller [ 12/Oct/15 7:57 PM ]

I don't want this to get applied as is so moving to incomplete for now.

Comment by Alex Miller [ 12/Oct/15 8:00 PM ]

You know this could be due to direct linking - calls inside the clojure code to other clojure code are now direct linked (static) calls as of 1.8.0-alpha3 and can't be redef'ed.

Comment by Alex Miller [ 12/Oct/15 8:03 PM ]

Yeah, if you turn direct linking off the test passes.

Comment by Jason Whitlark [ 13/Oct/15 1:06 PM ]

Fixed test to work with direct linking.

Comment by Jason Whitlark [ 10/Nov/15 12:58 AM ]

This is ready to go. Do I need to do anything to get it back in the queue?

Comment by Alex Miller [ 10/Nov/15 7:10 AM ]

We've missed the window for 1.8 but it's in the queue for 1.9.





[CLJ-1385] Docstrings for `conj!` and `assoc!` should suggest using the return value; effect not always in-place Created: 16/Mar/14  Updated: 09/Nov/15

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

Type: Enhancement Priority: Minor
Reporter: Pyry Jahkola Assignee: Alex Miller
Resolution: Unresolved Votes: 2
Labels: collections, docstring, ft

Attachments: Text File CLJ-1385-reword-docstrings-on-transient-update-funct-2.patch     Text File CLJ-1385-reword-docstrings-on-transient-update-funct.patch    
Patch: Code
Approval: Vetted

 Description   

The docstrings of both `assoc!` and `conj!` say "Returns coll.", possibly suggesting the transient edit happens (always) in-place, `coll` being the first argument. However, this is not the case and the returned collection should always be the one that's used.

Approach: Replace "Returns coll." with "Returns an updated collection." in `conj!`, `assoc!`, `pop!` docstrings.

Patch: CLJ-1385-reword-docstrings-on-transient-update-funct-2.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 16/Mar/14 8:49 AM ]

When modifying transient collections, it is required to use the collection returned from functions like assoc!. The ! here indicates its destructive nature. The transients page (http://clojure.org/transients) describes the calling pattern pretty explicitly: "You must capture and use the return value in the next call."

I do not agree that we should be guiding programmers away from using functions like assoc! – transients are used as a performance optimization and using assoc! or conj! in a loop is often the fastest version of that. However I do think it would be helpful to make the docstring more explicit.

Comment by Gary Fredericks [ 05/Apr/14 10:23 AM ]

Alex I think you must have misread the ticket – the OP is suggesting guiding toward using the return value of assoc!, not avoiding assoc! altogether.

And the docstring is not simply inexplicit, it's actually incorrect specifically in the case that the OP pointed out. conj! and assoc do not return coll at the point where array-maps transition to hash-maps, and the fact that they do otherwise is supposed to be an implementation detail as far as I understand it.

Comment by Alex Miller [ 05/Apr/14 11:55 AM ]

@Gary - you're right, I did misread that.

assoc and conj both explicitly say "return a new collection" whereas assoc! and conj! say "Returns coll." I read that as "returns the modified collection" without regard to whether it's the identical instance, but I can read it your way too.

Would saying "Returns updated collection." transmit the right idea? Using "collection" instead of "coll" removes the concrete tie to the variable and "updated" hints more strongly that you should use the return value.

Comment by Pyry Jahkola [ 05/Apr/14 12:47 PM ]

@Alex, that update makes it sound right to me, FWIW.

Comment by Gary Fredericks [ 05/Apr/14 2:37 PM ]

Yeah, I think that's better. Thanks Alex. I'd be happy to submit a patch for that but I'm assuming patches are too heavy for this kind of change?

Comment by Andy Fingerhut [ 06/Apr/14 3:35 PM ]

Patches are exactly what has been done in the past for this kind of change, if it is in a doc string and not on the clojure.org web page.

Comment by Alex Miller [ 06/Apr/14 4:13 PM ]

Yup, patch desired.

Comment by Gary Fredericks [ 06/Apr/14 5:32 PM ]

Glad I asked.

Patch is attached that also updates the docstring for pop! which had the same issue, though arguably it's less important since afaik pop! does always return the identical collection (but I don't think this is part of the contract).

Comment by Andy Fingerhut [ 06/Aug/14 2:14 PM ]

Patch CLJ-1385-reword-docstrings-on-transient-update-funct.patch dated Apr 6 2014 no longer applies to latest Clojure master cleanly, due to some changes committed earlier today. I suspect it should be straightforward to update the patch to apply cleanly, given that they are doc string changes, but there may have been doc string changes committed to master, too.

Comment by Gary Fredericks [ 06/Aug/14 3:04 PM ]

Attached a new patch.

Comment by Rich Hickey [ 09/Oct/15 8:04 AM ]

I think it could be clearer still, since we want people to know the original coll might have been affected and returned, and the return must be used for subsequent calls. I think some of the language from the transients page should make it into these docstrings.

Comment by Andy Fingerhut [ 24/Oct/15 2:25 PM ]

Would it be correct to say that the collection passed into pop! conj! assoc! etc. has undefined contents after the operation completes, and only the return value has defined contents?

That kind of strong wording may get people's attention.

Comment by Alex Miller [ 24/Oct/15 9:07 PM ]

I'm working on this.





Generated at Sat Feb 13 06:59:09 CST 2016 using JIRA 4.4#649-r158309.