<< Back to previous view

[DPRIMAP-10] doc confusion Created: 08/Jan/16  Updated: 10/Jan/16  Resolved: 10/Jan/16

Status: Closed
Project: data.priority-map
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: William Edward La Forge Jr Assignee: Mark Engelberg
Resolution: Not Reproducible Votes: 0
Labels: None
Environment:

none



 Description   

The way contains? is described, especially the similarity between contains? and get, one gets the distinct impression that contains? is really contains-key?, but it is not.

I found (contains? pm key) was failing, while (get pm key) was working fine.



 Comments   
Comment by William Edward La Forge Jr [ 08/Jan/16 4:25 PM ]

I should say that I'm refering to the readme.

You can test whether an item is in the priority map:

user=> (contains? p :a)
true

user=> (contains? p :g)
false
It is easy to look up the priority of a given item, using any of the standard map mechanisms:

user=> (get p :a)
2

user=> (get p :g 10)
10

user=> (p :a)
2

user=> (:a p)
2

Comment by Mark Engelberg [ 09/Jan/16 3:32 AM ]

`contains?` is meant to be semantically contains-key?, which is exactly the way contains? works for regular maps.

If you've found a counterexample, that would be a bug, so please show the counterexample you found.

Comment by William Edward La Forge Jr [ 09/Jan/16 12:13 PM ]

Unable to reproduce in a small code snippet:

(ns notify.priority-testapi
(:require [clojure.data.priority-map :refer :all]))

(def p (priority-map-keyfn first :a [2 0] :b [1 0]))

(println (contains? p :c)) ; -> false

Please close.





[DPRIMAP-8] Add clojurescript support Created: 05/May/15  Updated: 29/Nov/15

Status: Open
Project: data.priority-map
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Daniel Compton Assignee: Mark Engelberg
Resolution: Unresolved Votes: 0
Labels: None


 Description   

With the upcoming release of Clojure 1.7 and reader conditionals, is there interest in adding ClojureScript support? There is already a port at https://github.com/tailrecursion/cljs-priority-map. Alan Dipert may be interested in donating that to Clojure Core, otherwise I would be interested in working on a port.



 Comments   
Comment by Mark Engelberg [ 07/May/15 4:14 AM ]

I agree this is worthwhile, and working off of the existing port might help considerably. Do you want to take the lead in asking Alan Dipert and figuring out how to use the reader conditionals effectively to merge the two?

Comment by Tim Visher [ 16/Nov/15 4:00 PM ]

Are we still interested in doing this?

Comment by Mark Engelberg [ 16/Nov/15 11:17 PM ]

I am still supportive of the idea, but don't have time to do it myself right now.

Comment by Tim Visher [ 17/Nov/15 4:00 PM ]

Awesome. I'm digging in to this now. I just opened a different issue to try to get line endings together. http://dev.clojure.org/jira/browse/DPRIMAP-9

Comment by Tim Visher [ 17/Nov/15 5:24 PM ]

What would we like to do as far as clojure version support? If we were ok with dropping support for version prior to 1.7 then I could use `cljc` to target both platforms. If we want to maintain support for prior versions then I'll have to use a different approach.

Thoughts?

Comment by Mark Engelberg [ 17/Nov/15 11:52 PM ]

I said in my earlier comment that, "I think it is worthwhile," and that comment was driven entirely by the sense that "cljc is what modern Clojure libs are expected to do". But the more I think about it the more I realize that it's not entirely clear what we gain by merging the clojure and clojurescript versions, especially since this code is almost exclusively about implementing protocols and interfaces that are completely different between Clojure and Clojurescript, so there is very little code in common. It's also unclear, since the code has so little in common, that it will be any easier to maintain or extend the code in one combined file versus two separate files. On top of that, priority map has changed very little in the past several years since its creation, and will likely have very few changes in the future.

Therefore, I'm starting to feel skeptical that it is worth the effort and the risk of introducing bugs, but to the extent that you're interested in exploring this issue, I think cljc is the way to go.

So before you dive into it, let me ask you your opinion: what are the gains of mashing the two files together into one cljc file?

Comment by Tim Visher [ 26/Nov/15 6:58 AM ]

I think it's worthwhile if only because that would give 'official' contrib support to clojurescript. I'm mostly interested in this because I need a priority queue in a project I'm on and I'd also like to experiment with cljc. So this is partly a learning exercise and partly a practical effort.

That said I am quite surprised at how different the type declarations are. I was assuming I'd be able to just swap out some types using #? but the implementations are quite different. I can't decide yet how much of it I'll be able to unify, but I'm hoping it's quite a bit.

So I guess whether you accept it or not I'd like to go through the exercise.

I assume implicit in what you've said above is the acceptance of a minimum supported version of 1.7?

Comment by Mark Engelberg [ 28/Nov/15 4:07 PM ]

Yes, I'm fine with a minimum supported version of 1.7.

Comment by Alex Miller [ 29/Nov/15 9:57 PM ]

FYI, currently our Hudson CI system that is used to do builds of contrib projects cannot build cljc projects. To build, a new version of clojure-maven-plugin is required, which requires Maven 3 for some of the required plugins, which is not supported in the old version of Hudson we're using.

This is fairly high in my priority list as we are already fighting this issue for test.check (and it's only a matter of time before it's needed for others as well). But you might want to hold off till we've addressed this issue.

It is possible to manually build and release projects without the CI server (and I've done this for test.cehck), but it's no picnic.





[DPRIMAP-9] Files are using mixed line endings Created: 17/Nov/15  Updated: 17/Nov/15  Resolved: 17/Nov/15

Status: Closed
Project: data.priority-map
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Tim Visher Assignee: Mark Engelberg
Resolution: Completed Votes: 0
Labels: None
Environment:

All


Attachments: Text File data.priority-map_line-endings.patch    
Patch: Code

 Description   

The file are currently using mixed line endings which doesn't seem terribly desirable. I'd vote to standardize on unix?



 Comments   
Comment by Mark Engelberg [ 17/Nov/15 11:09 PM ]

There is a fundamental problem here:
1. Clojure contrib is a patch-only contribution system.
2. Git's patch system doesn't work for cross-platform teams.

The issue is that git screws around with line endings in weird ways when checking things in and out. In theory, what it is supposed to do on Windows systems is, upon checking out a file, converting all the files to CRLF, and when checking them in, storing them as LF.

This means that if you are on a Mac and I'm on Windows, the file checked-out on your file system looks different from the file checked-out on mine (completely different line endings). So when you give me a patch as a diff on your system, it won't apply on my computer. So I have no way to apply your patches.

This has made it extremely difficult for me to support my contrib libraries. In the past, I've resorted to manually copying over changes to my version of the file based on the patch. But for a large number of changes, that's impractical. So this is a problem we need to solve, I think, before we can meaningfully collaborate on this. I had forgotten about this issue until you brought up the point about line endings.

Any ideas on how to make cross-platform patches work would be welcome.

(P.S. I have no idea how the CRLFs got in there in the first place as git is supposed to remove them upon check-in. Perhaps the file was created back when git had different default behavior.)

Comment by Mark Engelberg [ 17/Nov/15 11:34 PM ]

I figured out the magical incantation to make git apply the patch successfully. I applied the patch and checked it in, so now you can proceed with your other modifications.





[DPRIMAP-7] Remove usage of clojure.test from the main namespace Created: 07/Aug/14  Updated: 17/Mar/15  Resolved: 17/Mar/15

Status: Resolved
Project: data.priority-map
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Alexander Yakushev Assignee: Mark Engelberg
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-Remove-use-of-clojure.test.patch    
Patch: Code

 Description   

The ns declaration of priority_map.clj contains unfiltered (:use clojure.test). It doesn't hurt when it is there until you AOT-compile the library which leads to compiling clojure.test as well (which is clearly useless there).



 Comments   
Comment by Alexander Yakushev [ 17/Mar/15 3:11 AM ]

Bump, can anyone fix this? It's really trivial.

Comment by Mark Engelberg [ 17/Mar/15 2:48 PM ]

Fixed in 0.0.6

Comment by Alexander Yakushev [ 17/Mar/15 2:51 PM ]

Thank you!





[DPRIMAP-6] (empty ...) forgets the comparator Created: 05/Sep/13  Updated: 13/Nov/13  Resolved: 13/Nov/13

Status: Resolved
Project: data.priority-map
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Jamie Brandon Assignee: Mark Engelberg
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File dprimap-6-v1.txt    
Patch: Code and Test

 Description   

The current implementation is:

(empty [this] pm-empty)

It should be:

(empty [this] (pm-empty-by (.comparator priority->set-of-items)))

This allows eg:

(into (empty pm) (filter f pm))

It also ensures that clojure.walk works correctly.



 Comments   
Comment by Andy Fingerhut [ 07/Sep/13 8:06 PM ]

Patch dprimap-6-v1.txt adds tests to verify that empty preserves not only the comparator, but also the metadata of the priority map. empty preserves the metadata of all other Clojure collections for which it is implemented.

It also modifies empty to pass these new tests.

Comment by Andy Fingerhut [ 13/Nov/13 3:41 PM ]

Mark Engelberg independently discovered and fixed this issue some time ago.





[DPRIMAP-4] There is one reflection warning in data.priority-map when invoking .equiv Created: 25/Apr/13  Updated: 13/Nov/13  Resolved: 13/Nov/13

Status: Resolved
Project: data.priority-map
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Mark Engelberg
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File dprimap-4-eliminate-equiv-reflection-patch-v1.txt    
Patch: Code

 Description   

Reflection warning, clojure/data/priority_map.clj:215:19 - call to equiv can't be resolved.

That line of source is this one inside the deftype for PersistentPriorityMap:
(equiv [this o] (.equiv item->priority o))



 Comments   
Comment by Andy Fingerhut [ 28/Apr/13 2:50 AM ]

Patch dprimap-4-eliminate-equiv-reflection-patch-v1.txt dated Apr 28 2013 eliminates an occurrence of reflection in the call to .equiv by adding a type hint.

Comment by Andy Fingerhut [ 13/Nov/13 3:39 PM ]

Mark Engelberg independently discovered and fixed this issue some time ago.





[DPRIMAP-5] Add support for subseq, rsubseq Created: 28/Apr/13  Updated: 28/Apr/13

Status: Open
Project: data.priority-map
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Mark Engelberg
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File dprimap-5-add-support-for-subseq-rsubseq-patch-v1.txt    
Patch: Code and Test

 Description   

This depends upon some kind of change to clojure.core, either to the Sorted interface or the implementation of subseq and rsubseq, as discussed in this thread: http://groups.google.com/group/clojure-dev/browse_thread/thread/fdb000cae4f66a95

There is a ticket CLJ-428 for these proposed clojure.core changes.



 Comments   
Comment by Andy Fingerhut [ 28/Apr/13 2:25 AM ]

Patch dprimap-5-add-support-for-subseq-rsubseq-patch-v1.txt dated Apr 28 2013 depends upon Mark Engelberg's "inclusive" patch for CLJ-428, or something similar, being accepted into clojure.core.

It mostly just uncomments some code that Mark wrote, but also slightly modifies the implementation of seqFrom to take advantage of that new parameter.





[DPRIMAP-3] Add more developer info and Markdown markup to README Created: 02/Apr/13  Updated: 02/Apr/13

Status: Open
Project: data.priority-map
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Mark Engelberg
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File dprimap-readme-touchups-patch-v1.txt    
Patch: Code

 Description   

Just a bit more tweaking of the README.



 Comments   
Comment by Andy Fingerhut [ 02/Apr/13 1:19 PM ]

Patch dprimap-readme-touchups-patch-v1.txt

To see how these changes look on Github, see my forked version here:

https://github.com/jafingerhut/data.priority-map/tree/cleanup-readme





[DPRIMAP-2] data.priority-map has no artifacts information in the README Created: 14/Sep/12  Updated: 23/Nov/12  Resolved: 23/Nov/12

Status: Resolved
Project: data.priority-map
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Mark Engelberg
Resolution: Completed Votes: 0
Labels: None


 Description   

data.priority-map README has no artifacts information and does not conform to the Contrib README standards in any way. Because clojure-dev is a closed group, I am filing it here



 Comments   
Comment by Mark Engelberg [ 23/Nov/12 3:01 AM ]

Updated README





[DPRIMAP-1] Implement java.lang.Iterable Created: 04/Sep/12  Updated: 09/Sep/12  Resolved: 09/Sep/12

Status: Resolved
Project: data.priority-map
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Alan Malloy Assignee: Sean Corfield
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File iterable.patch    
Patch: Code

 Description   

Clojure collections are expected to implement all relevant java collection interfaces, and things will quietly and subtly break if any are missed. In this case, the reducers library assumes all instances of java.util.Map implement java.lang.Iterable, and since this one doesn't it can't reduce over us, even inefficiently.



 Comments   
Comment by Sean Corfield [ 09/Sep/12 2:30 AM ]

The patch from Alan did not apply but it was only a two line addition so I did that manually. Passes current tests but there are no new tests for Iterable (yet).





Generated at Tue Feb 09 05:47:33 CST 2016 using JIRA 4.4#649-r158309.