-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
base: master
Are you sure you want to change the base?
Interfaces #25
Conversation
431b103
to
2c26948
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
||
(p.types/definterface+ MethodCombination |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
methodical.interface
is compiled ahead of time. If you change this namespace you should reload jvm.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))
- 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.))
- 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
There was a problem hiding this comment.
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
src/methodical/interface.clj
Outdated
[^MethodTable method-table qualifier dispatch-val method] | ||
(.removeAuxMethod method-table qualifier dispatch-val method)) | ||
|
||
(definterface Dispatcher |
There was a problem hiding this comment.
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
?)
There was a problem hiding this comment.
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.
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 |
a90b736
to
2f86cdd
Compare
I want to get ClojureScript support working in the near future. I'm switching from |
I replaced
definterface+
withdefinterface
for further clojurescript support #20.UPD: Also I replaced
deftype+
withdeftype
.Helper functions are not inlined like potemkin do because I not see any performance impact.
So we can do it: