Skip to content

Add try/catch when reading EDN data #256

New issue

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

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

Already on GitHub? # to your account

Merged
merged 3 commits into from
Jun 5, 2019

Conversation

andersenleo
Copy link
Contributor

Fixes #255

Since the artifacts/get-clojars-artifacts! function is written in a way that isn't easily testable I opted out on tests. I would need to refactor the code a bit more to make it possible to write a test for this. Let me know if you'd like me to do that.

The fix is simply to use keep instead of map - and swallow any exception on edn/read-string.

Ignores unreadable entities.
@benedekfazekas
Copy link
Member

A test would be nice I think. Please also add a one liner to the changelog. Thanks again, really appreciated.

@andersenleo
Copy link
Contributor Author

Moved the "reading" to a separate function and wrote a minimal test for verifying that. Let me know if you'd like to restructure things differently.

@benedekfazekas benedekfazekas merged commit 8f8647d into clojure-emacs:master Jun 5, 2019
@benedekfazekas
Copy link
Member

good work @andersenleo! will release a new snapshot in the evening.

@andersenleo
Copy link
Contributor Author

@expez - the edn/read-string function is called when the lazy-seq returned from get-clojars-artifacts! is evaluated further down the line. The try/catch that was in place would only return [] if the connection to Clojars was down (an error triggered before call to line-seq). And either way -- returning an empty seq would still make the feature unusable (if the artifact seq would have been eagerly evaluated).

@expez
Copy link
Member

expez commented Jun 5, 2019

Thanks @andersenleo I eventually got that and deleted my comment hoping nobody would've seen it! :p

Great job on this! 👍

@benedekfazekas
Copy link
Member

new snapshot version is on clojars

@andersenleo
Copy link
Contributor Author

Lovely - it seems to work. I'm no Emacs expert, but adding

  (setq cider-jack-in-lein-plugins '(("refactor-nrepl" "2.4.1-SNAPSHOT" :predicate cljr--inject-middleware-p)
                                     ("cider/cider-nrepl" "0.22.0-beta1")))

works fine. Should be an easier way to override the refactor-nrepl version I guess? Suggestions are welcome :). Any idea when a "proper" release could be made available?

@benedekfazekas
Copy link
Member

looks legit to me. no release is planned atm. nothing blocking one either afaik

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cljr-add-project-dependency blows up when parsing Clojar artifacts
3 participants