Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for NamedList conversion regression #4

Closed
wants to merge 1 commit into from

Conversation

pbostrom
Copy link

  • Commit 62ae93e introduced a regression when converting NamedList
    objects
  • In previous version, covert-named-list returned a map with string
    keys converted to keywords
  • The new ->clojure multi method will return a vector of 2-element
    vectors when called on a NamedList (string keys not converted)
  • Unit test provided to illustrate issue

- Commit 62ae93e introduced a regression when converting NamedList
  objects
- In previous version, covert-named-list returned a map with string
  keys converted to keywords
- The new ->clojure multi method will return a vector of 2-element
  vectors when called on a NamedList (string keys not converted)
- Unit test provided to illustrate issue
@mwmitchell
Copy link
Owner

Sorry for the delay @pbostrom! I had to think about this a bit. The reason it went from a hash-map to a vector of pairs, was to preserve the order of the NamedList items. For example, the facet response list/object is ordered by "hits". If that's converted into a map then the order is lost and it needs to be resorted if you want to sort by hits.

Is this causing problems for your app? Is it at all feasible to do something like you've done in this patch to your app via (into {} ...)?

@pbostrom
Copy link
Author

pbostrom commented Sep 8, 2014

I did spend some time trying to do this conversion in our app instead, but I kept running into problems. It turns out that it was probably related to the move to solr 4.9.0. So we still have some things to sort out that this PR would not solve anyway. For now we just forked flux, keeping the solr 4.6.0 dependency and the old NamedList format, but I would eventually like to sync back up, so ultimately this PR will probably be unnecessary.

@mwmitchell
Copy link
Owner

OK thanks @pbostrom

@mwmitchell mwmitchell closed this Sep 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants