Skip to content

Conversation

tatut
Copy link
Contributor

@tatut tatut commented Jan 16, 2019

Fallback to "application/octet-stream" if a content-type cannot be determined for the file/url.

Don't show the content of unrecognized files, but return a description "#binary[location=,size=".

Added test in slurp_test.clj to test unrecognized file.

@codecov
Copy link

codecov bot commented Jan 16, 2019

Codecov Report

Merging #587 into master will decrease coverage by 0.04%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #587      +/-   ##
==========================================
- Coverage   76.72%   76.68%   -0.05%     
==========================================
  Files          38       38              
  Lines        2389     2393       +4     
  Branches      138      140       +2     
==========================================
+ Hits         1833     1835       +2     
  Misses        418      418              
- Partials      138      140       +2
Impacted Files Coverage Δ
src/cider/nrepl/middleware/content_type.clj 33.33% <0%> (ø) ⬆️
src/cider/nrepl/middleware/slurp.clj 60.52% <84.61%> (-0.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfbfa3e...7cd7ec6. Read the comment docs.

@cichli cichli requested a review from arrdem January 19, 2019 01:17
@cichli
Copy link
Member

cichli commented Jan 19, 2019

Thanks for the contribution! Please will you rebase to master? It's just a small conflict. Note that this also conflicts with #589.

The changes look fine to me – @arrdem do you mind double-checking?

Copy link
Contributor

@arrdem arrdem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'd like to see this all get better factored so that users can opt into different sets of binary file handling behavior. Not sure this is a good universal fallback, but I do believe it's better.

Thanks for the patch!

@cichli cichli merged commit 734660f into clojure-emacs:master Jan 21, 2019
@cichli
Copy link
Member

cichli commented Jan 21, 2019

Cheers @tatut @arrdem!

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.

3 participants