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

Use a multimethod for content-type dispatching #548

Closed
wants to merge 2 commits into from

Conversation

plexus
Copy link
Contributor

@plexus plexus commented Oct 10, 2018

Make the content-type middleware that handles inlining of File/URL/Image
extensible through a multimethod. This makes it possible for developers to add
handling of custom types.

Regular Clojure values can be given a :type metadata to make them
distinguishable. Other types can dispatch on the class.

  • The commits are consistent with our contribution guidelines
  • You've added tests to cover your change(s)
  • All tests are passing
  • The new code is not generating reflection warnings
  • You've updated the README (if adding/changing middleware)

@plexus
Copy link
Contributor Author

plexus commented Oct 10, 2018

I want to see if I can get a few tests on this, but it might take me a few days.

@bbatsov
Copy link
Member

bbatsov commented Oct 10, 2018

The implementation looks good to me. You might mention something about this in the readme and the changelog.

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! Thanks for cleaning this up @plexus.

@plexus
Copy link
Contributor Author

plexus commented Oct 11, 2018

Thanks for looking at this. I'll be on the road a lot the coming week but I do plan to still add some stuff here so don't merge yet.

Note to self:

  • README
  • Changelog
  • CI fails?
  • Add tests

@plexus plexus force-pushed the content-type-dispatch branch from f5f9023 to da53829 Compare November 23, 2018 08:29
@codecov
Copy link

codecov bot commented Nov 23, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@ca08ea4). Click here to learn what that means.
The diff coverage is 38.09%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #548   +/-   ##
=========================================
  Coverage          ?   72.97%           
=========================================
  Files             ?       36           
  Lines             ?     2442           
  Branches          ?      142           
=========================================
  Hits              ?     1782           
  Misses            ?      518           
  Partials          ?      142
Impacted Files Coverage Δ
src/cider/nrepl/middleware/content_type.clj 47.72% <38.09%> (ø)

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 ca08ea4...da53829. Read the comment docs.

@bbatsov
Copy link
Member

bbatsov commented Dec 21, 2018

@plexus I plan to cut 0.19 in the next few days. Let me know if you want me to merge this for it or not.

@plexus
Copy link
Contributor Author

plexus commented Dec 21, 2018 via email

@bbatsov
Copy link
Member

bbatsov commented Oct 3, 2019

One year later...

I think we've got a new CI and that's it's green! 😉

@plexus
Copy link
Contributor Author

plexus commented Oct 4, 2019

I swear I think about this PR at least once a month 🙃

@arrdem
Copy link
Contributor

arrdem commented Oct 4, 2019

Careful, you're gonna make me dust off my Clojure setup just to free myself from the GH notifications ;)

@bbatsov
Copy link
Member

bbatsov commented Oct 18, 2021

@plexus This one needs to be rebased. Perhaps we will finally merge it this year! :D

Make the content-type middleware that handles inlining of File/URL/Image
extensible through a multimethod. This makes it possible for developers to add
handling of custom types.

Regular Clojure values can be given a `:type` metadata to make them
distinguishable. Other types can dispatch on the class.
@plexus plexus force-pushed the content-type-dispatch branch from da53829 to 57bde4d Compare January 1, 2022 20:45
@plexus
Copy link
Contributor Author

plexus commented Jan 1, 2022

The CI woes never end 🤣

Cloning git repository
Cloning into '.'...
Warning: Permanently added the ECDSA host key for IP address '140.82.114.4' to the list of known hosts.
Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

In any case, rebased, and adding a CHANGELOG entry and some info in the docs.

@bbatsov
Copy link
Member

bbatsov commented Jan 1, 2022

In any case, rebased, and adding a CHANGELOG entry and some info in the docs.

As for the CI - I like the idea of switching to GHA more and more each day. :D

Further document the content-type middleware.
@plexus
Copy link
Contributor Author

plexus commented Jan 1, 2022

2022-01-01_161255_Firefox

Last used four weeks ago... did Circle forget our key?

@plexus
Copy link
Contributor Author

plexus commented Jan 1, 2022

Maybe it's to do with me pulling from my own repo? let me try to open a fresh PR.

@plexus plexus mentioned this pull request Jan 1, 2022
5 tasks
@plexus
Copy link
Contributor Author

plexus commented Jan 1, 2022

Closing this in favor of #742

@plexus plexus closed this Jan 1, 2022
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