<< Back to previous view

[DZIP-1] tag= doesn't work with records Created: 03/Feb/12  Updated: 28/Apr/16  Resolved: 30/Mar/12

Status: Closed
Project: data.zip
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Justin Kramer Assignee: Aaron Bedra
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-fix-clojure.data.zip.xml-tag-to-work-with-records.patch    
Patch: Code and Test
Approval: Ok

 Description   

clojure.data.zip.xml/tag= throws when given a record instead of a map, even if it has fields for tag, attrs, and content. Reason: tag= calls (node :tag) instead of (:tag node).

This was uncovered while trying to use data.zip with Ryan Senior's new data.xml, which produces Element records.

Patch with fix and test attached.



 Comments   
Comment by Aaron Bedra [ 30/Mar/12 4:08 PM ]

Thanks! I just gave it quick once over and I think it should cover things nicely. I will take a closer look this evening and if things are proper i'll cut a new version and release right away. This is an issue that bit me just last week that I never got around to reporting.

Comment by Aaron Bedra [ 30/Mar/12 4:48 PM ]

This has been committed and a release 0.1.1 has been pushed. It is staged to Sonatype now and should hit Maven Central within a few hours.

Comment by Alex Miller [ 28/Apr/16 10:55 AM ]

applied long time ago, closing





[DZIP-5] Make it compatible with ClojureScript Created: 01/Nov/15  Updated: 28/Apr/16  Resolved: 28/Apr/16

Status: Closed
Project: data.zip
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Artem Yarulin Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: File data.zip-cljs-added-2.diff     File data.zip-cljs-added-3.diff     File data.zip-cljs-added.diff    

 Description   

It would be really cool to have this library for ClojureScript as well. Currently ReactNative is rising and ClojureScript which has full support of it, will benefit from this lib.

I'm new to the Clojure world, but for me it looks like we can simply rename src/*/.clj files to *.cljc and support both environments.

What do you think? Is this a right approach?



 Comments   
Comment by Artem Yarulin [ 01/Nov/15 1:32 AM ]

Forgot to mention: First http://dev.clojure.org/jira/browse/DZIP-4 has to be resolved

Comment by Artem Yarulin [ 01/Nov/15 1:04 AM ]

OK, this is not that easy:
For example https://github.com/clojure/data.zip/blob/master/src/main/clojure/clojure/data/zip/xml.clj#L41-43:

(.replaceAll
^String (apply str (xml-> loc zf/descendants zip/node string?))
(str "\\s" (char 160) "+") " "))

Has to be replaced with clojure.string/replace and who knows what else.

Can you point me a good example of Clojure/ClojureScript library? Obviously we have to run all the tests for CLJS version as well, but in my projects I'm using projects like doo for that, not sure that this is a right approach.

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

Changing the files to cljc means that this library would then require Clojure 1.7. Generally we try not to be eager about having such a recent release be the minimum requirement.

There are several contrib projects that have both clojure and clojurescript support although the build process is kind of a pain for all of them. core.logic is one example.

Comment by Artem Yarulin [ 04/Nov/15 8:16 AM ]

Can we make an exception from this rule? Last commit to this library was on May 21, 2014, so in most cases this library is done and doesn't require any new functionality.

With *.cljc files it will be so much easier to create universal libraries.

Comment by Alex Miller [ 04/Nov/15 8:25 AM ]

I'm not ready to make 1.7 a minimum requirement for this library yet. While doing a split build with duplicated code for clj and cljs would be more complicated, I think that is preferable for the time being.

Comment by Artem Yarulin [ 04/Nov/15 8:36 AM ]

Basically:

1 create a folder main/clojurescript/clojurescript/data
2 copy files from Clojure and change ext to cljs
3 change namespace from clojure.data.zip.xml to clojurescript.data.zip.xml to refer folder change
4 make needed changes (remove any interop to Java world, like .replaceAll)
5 change pom.xml to lein compatible project.clj and build CLJS version using cljsbuild (like in core.logic)
6 make the same for tests (folder creation + copy + rename + remove interop)
7 run tests like in core.logic (https://github.com/clojure/core.logic/blob/master/script/test)

Is this a right approach?

Comment by Alex Miller [ 04/Nov/15 9:09 AM ]

Yes, that's basically it. Based on the direction of other libraries, I think it would be better to leave the namespaces the same (for future cljc-ification).

Comment by Artem Yarulin [ 04/Nov/15 9:10 AM ]

Ack, thx. I'll try to do this during this weekend

Comment by Artem Yarulin [ 09/Nov/15 3:12 AM ]

Here the patch which adds CLJS support.

While I followed the workflow that we discussed before I have couple of questions:

  • Could you please verify project.clj and pom.xml. I don't know what pom.xml is used for, but I've tried to merge all the information from it to project.clj. Also I increased a version to 0.1.3
  • zip.cljs and xml.cljs is pretty much the same as clojure version. Only one change is replacing .replaceAll with clojure.string/replace function call
  • xml_test.cljs - as data.xml not yet ported to CLJS I've replaced test atom with already created map representation of xml doc
  • I found a script/test in core.logic and decided to include it here as well as I think it could be used for CI. Currently the easiest way to test it is: lein test && lein cljsbuild once adv && node resources/tests.js
Comment by Alex Miller [ 09/Nov/15 8:29 AM ]

I won't have time to look at the patch right now, but answering your questions:

  • pom.xml is the Maven build - this is what actually produces the release artifact (the project.clj is a convenience but the pom.xml is actually the more important project file). Don't modify the version - the release process does this automatically.
  • is it possible to also have the .clj version use clojure.string/replace instead of .replaceAll?
  • we don't currently run lein at all during the release build, but it is useful for verification, so happy to have the script/test.
Comment by Artem Yarulin [ 09/Nov/15 8:57 AM ]

No problem - whenever you have time.

1) I'll revert version
2) Sure thing - I'll make the same change
3) kk

I'll create a new patch then

Comment by Artem Yarulin [ 10/Nov/15 2:04 PM ]

Fixed all the issues you mentioned:
1) Version reverted to the original one
2) string/replace used in clojure version as well

BTW - lein re-generated pom.xml file and I included changed file as well. Not sure if this is a right thing

Comment by Alex Miller [ 10/Nov/15 2:06 PM ]

Please don't re-generate the pom file - it is the primary build definition (the project.clj is secondary).

Comment by Artem Yarulin [ 11/Nov/15 9:49 AM ]

Changes to pom.xml removed

Now it should be pretty much done

Comment by Artem Yarulin [ 22/Nov/15 12:08 AM ]

Sorry for the annoyance, don't know how fast things are happening here.

Is there anything I can do to push this patch forward? Create example project, more docs, etc.?

Thanks

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

Nope, just waiting on me to have time.

Comment by Artem Yarulin [ 07/Jan/16 9:18 AM ]

Just passing by, reminding that this exists...

Comment by Alex Miller [ 28/Apr/16 10:55 AM ]

I have added this (with some additional changes) for the next release of data.zip.





[DZIP-3] Not returning expected result for 'tag=' Created: 17/Apr/14  Updated: 28/Apr/16  Resolved: 28/Apr/16

Status: Closed
Project: data.zip
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Frank Castellucci Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None
Environment:

:dependencies [[org.clojure/clojure "1.5.1"]
[org.clojure/data.zip "0.1.1"]]


Attachments: Text File 0001-DZIP-3-doesnt-match-on-first-node.patch    
Patch: Code and Test

 Description   

Setup:

(require '[clojure.xml :as xml])
(require '[clojure.zip :as zip])
(require '[clojure.data.zip.xml :as dzx])

(def s "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<result>\n <status>success</status>\n <message>success</message>\n <format>xml</format>\n <language>en</language>\n <ip>208.67.220.220</ip>\n <country_code>US</country_code>\n <country>United States</country>\n <region>California</region>\n <city>San Francisco</city>\n <latitude>37.7757</latitude>\n <longitude>-122.3952</longitude>\n <zip_code>94107</zip_code>\n <timezone>-08:00</timezone>\n <localtime>2014-04-17 06:52:45</localtime>\n</result>\n")

(def x (xml/parse (org.xml.sax.InputSource. (java.io.StringReader. s))))
(def z (zip/xml-zip x))

Works as expected:

(dzx/xml1-> z :ip dzx/text)
;; "208.67.220.220"

(dzx/xml1-> z :status dzx/text)
;; "success"

Doesn't seem to work as expected:

(dzx/xml1-> z :result)
;; nil   (expected something not nil)

(dzx/xml1-> z :result :status dzx/text)
;; nil   (expected "success")


 Comments   
Comment by Erik Assum [ 27/Apr/16 3:11 PM ]

Looking at the definition of tag=
https://github.com/clojure/data.zip/blob/master/src/main/clojure/clojure/data/zip/xml.clj#L26

it does some more logic (which I don’t understand) than both
attr=
https://github.com/clojure/data.zip/blob/master/src/main/clojure/clojure/data/zip/xml.clj#L21

and
text=
https://github.com/clojure/data.zip/blob/master/src/main/clojure/clojure/data/zip/xml.clj#L44

So I’m curious as to why tag= cannot be implemented as:

(defn tag= [tagname]
  (fn [loc]
    (= tagname (:tag (zip/node loc)))))
Comment by Erik Assum [ 27/Apr/16 3:11 PM ]

Looking at the definition of tag=
https://github.com/clojure/data.zip/blob/master/src/main/clojure/clojure/data/zip/xml.clj#L26

it does some more logic (which I don’t understand) than both
attr=
https://github.com/clojure/data.zip/blob/master/src/main/clojure/clojure/data/zip/xml.clj#L21

and
text=
https://github.com/clojure/data.zip/blob/master/src/main/clojure/clojure/data/zip/xml.clj#L44

So I’m curious as to why tag= cannot be implemented as:

(defn tag= [tagname]
  (fn [loc]
    (= tagname (:tag (zip/node loc)))))
Comment by Erik Assum [ 27/Apr/16 3:16 PM ]

A reason for this would be that the tests fail... but why?

Comment by Erik Assum [ 27/Apr/16 4:08 PM ]

So it seems like the problem is that you get an empty match if you try to match a tag on the first element in the xml:

(def atom2 (parse-str "<feed>
<foo>bar</foo></feed>"))

(deftest dzip-3-first-node
  (is (not= (xml-> atom2 :feed ) '()))) ; fail

(deftest dzip-3-second-node
  (is (= (xml-> atom2 :foo text) '("bar")))) ; pass
Comment by Erik Assum [ 27/Apr/16 4:41 PM ]

So, if you do

(def atom2 (parse-str "<feed>
<foo>bar</foo></feed>"))

(deftest dzip-3-first-node
  (is (not= (xml-> (clojure.data.zip/auto false atom2) :feed ) '())))

(deftest dzip-3-second-node
  (is (= (xml-> atom2 :foo text) '("bar"))))

the tests will pass. Whereas if you

(def atom2 (parse-str "<feed>
<foo>bar</foo></feed>"))

(deftest dzip-3-first-node
  (is (not= (xml-> (clojure.data.zip/auto false atom2) :feed ) '())))

(deftest dzip-3-second-node
  (is (= (xml-> (clojure.data.zip/auto false atom2) :foo text) '("bar"))))

zip-3-second-node will fail.

Comment by Erik Assum [ 27/Apr/16 11:58 PM ]

The problem was that tag= checked for a match either in the zippers node or in its
children. The patch solves this by first looking for a match in the node,
and if that fails, look for a match in the children.

Comment by Alex Miller [ 28/Apr/16 8:51 AM ]

Thanks, applied for next release!





[DZIP-4] Remove unused require of clojure.xml Created: 01/Nov/15  Updated: 01/Nov/15  Resolved: 01/Nov/15

Status: Closed
Project: data.zip
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Artem Yarulin Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: File patch.diff    
Patch: Code

 Description   

clojure.data.zip.xml has an unused require of clojure.xml which should be removed as clojure.xml is not available on ClojureScript.

Same as here: https://github.com/clojure/data.zip/pull/3



 Comments   
Comment by Alex Miller [ 01/Nov/15 7:07 AM ]

Fixed.





[DZIP-2] Empty file src/test/clojure/clojure/data/zip.clj should be removed Created: 15/Dec/13  Updated: 01/Nov/15  Resolved: 01/Nov/15

Status: Closed
Project: data.zip
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None


 Description   

It serves no purpose being in the library, since it is empty.

Secondarily, it inhibits the use of Leiningen with the project, because if one sets :source-paths ["src/main/clojure"] and :test-paths ["src/test/clojure"], its position in the file system implies that it should have a namespace of clojure.data.zip, the same as the source file src/main/clojure/clojure/data/zip.clj, and even causes (require '[clojure.data.zip :as z]) to throw an exception.



 Comments   
Comment by Aaron Bedra [ 27/Dec/13 2:16 PM ]

Sorry, I just saw this pop up in a Google search. I didn't get the JIRA notification. I will take a closer look soon.

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

Done





Generated at Sat Apr 30 23:16:45 CDT 2016 using JIRA 4.4#649-r158309.