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

Interfaces #25

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Interfaces #25

wants to merge 9 commits into from

Conversation

darkleaf
Copy link
Contributor

@darkleaf darkleaf commented Sep 12, 2019

I replaced definterface+ with definterface for further clojurescript support #20.
UPD: Also I replaced deftype+ with deftype.

Helper functions are not inlined like potemkin do because I not see any performance impact.

So we can do it:

#?(:clj
   (definterface Foo
     (fooBar []))
   :cljs
   (def Foo))

;; helper function
(defn foo-bar [^Foo foo]
  (.fooBar foo))

(deftype Bar []
  #?(:clj Foo
     :cljs Object)
  (fooBar [_] :ok))

(foo-bar (Bar.))

@codecov-io
Copy link

codecov-io commented Sep 12, 2019

Codecov Report

Merging #25 into master will decrease coverage by 0.14%.
The diff coverage is 91.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage   86.57%   86.43%   -0.15%     
==========================================
  Files          20       20              
  Lines         827      936     +109     
  Branches       48       57       +9     
==========================================
+ Hits          716      809      +93     
- Misses         63       70       +7     
- Partials       48       57       +9
Impacted Files Coverage Δ
src/methodical/impl/dispatcher/everything.clj 57.69% <0%> (ø) ⬆️
src/methodical/impl/combo/clojure.clj 62.5% <0%> (-37.5%) ⬇️
src/methodical/impl/combo/clos.clj 60% <0%> (ø) ⬆️
src/methodical/impl/combo/threaded.clj 94.11% <0%> (-3.67%) ⬇️
src/methodical/impl/cache/simple.clj 71.42% <100%> (ø) ⬆️
src/methodical/impl/multifn/standard.clj 81.81% <100%> (ø) ⬆️
src/methodical/impl/method_table/clojure.clj 70.58% <100%> (-29.42%) ⬇️
src/methodical/impl/standard.clj 93.75% <100%> (ø) ⬆️
src/methodical/impl/method_table/standard.clj 86.95% <100%> (ø) ⬆️
src/methodical/impl/multifn/cached.clj 86.36% <100%> (ø) ⬆️
... and 7 more

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 3f3b737...7ebbd0b. Read the comment docs.


(p.types/definterface+ MethodCombination
Copy link
Owner

Choose a reason for hiding this comment

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

I want to keep definterface+ and deftype+ for Clojure code since it makes REPL-based development so much nicer by not redefining types/protocols every time the namespace is reloaded.

How about creating some other macro somewhere, e.g. methodical.types/definterface+ that expands to a definterface+ form in :clj and definterface in :cljs? Then it's a simple matter of replacing p.types/definterface+ with m.types/definterface+ where it pops up

Copy link
Contributor Author

@darkleaf darkleaf Sep 13, 2019

Choose a reason for hiding this comment

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

  1. methodical.interface is compiled ahead of time. If you change this namespace you should reload jvm.
  2. definterface+ defines helper functions but in clojurescript we also need them.
(definterface+ Foo
  (foo-bar [this]))

;; will be expanded into

;; conditional loading
;; not implemented by me // UPD: implemented
;; https://github.com/ztellman/potemkin/blob/master/src/potemkin/types.clj#L239

;; https://github.com/ztellman/potemkin/blob/master/src/potemkin/types.clj#L251
(definterface Foo
  (foo_bar []))

;; helper function
;; https://github.com/ztellman/potemkin/blob/master/src/potemkin/types.clj#L276
(defn ^:inline ... foo-bar [^Foo foo]
  (.foo_bar foo))
  1. ClojureScript not implements interfaces. Look again please:
#?(:clj
   (definterface Foo
     (fooBar []))
   :cljs
   (def Foo)) ;; ### NAME JUST FOR TYPE HINTS ###

;; ### COMMON HELPER FUNCTION ###
(defn foo-bar [^Foo foo]
  (.fooBar foo))

(deftype Bar []
  #?(:clj Foo
     :cljs Object)   ;; ### OBJECT NOT INTERFACE ###
  (fooBar [_] :ok))

(foo-bar (Bar.))
  1. For repl development I can add simple macro for clj like definterface+ that not load interface if it already exists.
    https://github.com/ztellman/potemkin/blob/master/src/potemkin/types.clj#L239-L253

Copy link
Contributor Author

@darkleaf darkleaf Sep 13, 2019

Choose a reason for hiding this comment

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

I implemented defonceinterface for repl dev.
0e65c54

[^MethodTable method-table qualifier dispatch-val method]
(.removeAuxMethod method-table qualifier dispatch-val method))

(definterface Dispatcher
Copy link
Owner

Choose a reason for hiding this comment

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

instead of changing all the interfaces, wouldn't it be easier to change the one or two places where the methods are invoked directly (e.g. StandardMultiFn?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what you mean.

I just replaced definterface+ with definterface and replace lisp-case names with camelCase ones.

@camsaul
Copy link
Owner

camsaul commented Sep 21, 2019

Hey @darkleaf, sorry I haven't followed up on this lately. I've been pretty busy shepherding some new releases at work and haven't got a change to circle back to this. Can you show me what running tests for this will look like with ClojureScript? I don't have a ton of ClojureScript experience so I would like to play around with your PR a bit so I can understand it better

@darkleaf
Copy link
Contributor Author

Can you show me what running tests for this will look like with ClojureScript?

Currently I can't. Because methodical havely used vars and cljs have reduced vars implementation.
We should resolve #29 first.

But I write some cljc code in 7ebbd0b

@camsaul camsaul added the clojurescript support Issues that we have to tackle in order to get ClojureScript support working. label Jun 4, 2021
@camsaul
Copy link
Owner

camsaul commented Jun 12, 2021

I want to get ClojureScript support working in the near future. I'm switching from definterface+ to defprotocol in #71. I'll probably make the switch from deftype+ to plain deftype in the near future as well.

@tekacs tekacs mentioned this pull request Jan 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clojurescript support Issues that we have to tackle in order to get ClojureScript support working.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants