Fork me on GitHub
#cider
<
2018-05-05
>
dominicm05:05:57

Refactor nrepl has a locals op

bozhidar08:05:49

Using tools.analyzer. 🙂

bozhidar08:05:21

The problem with it is that overall complexity of the code rose a lot by adding it, not to mention some features there require parsing all project files which is pretty slow. That’s why I was wary of relying on it in CIDER itself. If it’s usage is limited to locals that’d be fine, though.

bozhidar12:05:48

(and there’s also the fact that to generate the AST you have to evaluate the code, which not everyone is expecting) 🙂

dominicm12:05:44

interesting thought: - apropos - test - ns ops - more? all have ns & var filtering designed in a custom ad-hoc way. It would be really useful to extend apropos to support search-ns being a filtered list based on whether it's a project ns or not, for example.

dominicm12:05:40

I wonder if it should be a DSL supporting and/or? Probably too complicated right?

dominicm12:05:49

yeah, no client would ever send that

dominicm12:05:13

Something quite short works surprisingly well

dominicm12:05:05

It's a good opportunity to have ns-vars return vars from multiple namespaces too, e.g. {:op "ns-vars", :ns-query {:filter-regexps [#"clojure\.core"]} Get all clojure core namespaces {:op "ns-vars", :ns-query {:exactly 'clojure.core}} Only clojure core {:op "ns-vars", :ns-query {:project? true}} Only vars in the current project {:op "ns-vars", :ns-query {}} All vars

dominicm12:05:35

Adding metadata support as requested for tests becomes more generally useful too!

bozhidar12:05:52

> all have ns & var filtering designed in a custom ad-hoc way. It would be really useful to extend apropos to support search-ns being a filtered list based on whether it’s a project ns or not, for example.

bozhidar12:05:13

That’s a very good point! Unfortunately the API evolved naturally (chaotically) and we didn’t give much thought to making it uniform. Actually I’ve been thinking about this for a while now both for orchard and cider-nrepl and I think it’s really important to work in this direction. The work’s not much (or complex), but would make the API usage way easier and consistent.

bozhidar12:05:42

Same goes for refactor-nrepl - many things there are wildly different from cider-nrepl and there’s no reason for this difference most of the time.

dominicm13:05:58

cider is organic growth, so is orchard 😛. I suppose there's rarely much motivation to do a sweeping "fix" like this.

dominicm13:05:07

If I make this change, I'm likely to cause downstream pain for you.

dominicm13:05:04

I could make it in a backwards compatible way, now that I think about it, only using ns-query if none of the "deprecated" attributes are there.

bozhidar13:05:07

I’m not very worried about this - updating the client usages doesn’t take much time. I’m willing to deal with that effort in the name of better future for everyone. 🙂

bozhidar13:05:15

Yeah, if you’re willing to make it work in backward compatible manner that’d be best, although ideally we should also send some deprecation warnings to the clients, so they authors would update them.

bozhidar13:05:35

I’m not fond of the approach of keeping around compatibility code forever.

dominicm13:05:20

Heresy! Accretion is the future.

dominicm13:05:50

Oh wow, I could totally kill test-ns, as it's just a way of doing (ns/list-vars {:ns-query {:exactly #{'clojure.core-test}} :test? true})

dominicm14:05:08

@benedek these changes would also work very nicely with your new op for listing tests

bozhidar14:05:40

Yep, that seems reasonable to me.

dominicm14:05:06

There's one little hack in here for special symbols, but otherwise this is looking to be a fairly clean abstraction.

dominicm14:05:38

I'm trying to figure out: A) Whether apropos has much value beyond sorting B) Whether to match the regex behaviour, or to extend it to allow a regex for both doc & name.

lmergen15:05:00

hey, i'm debugging an issue with emacs + saving files being slow, and i think cider is to blame -- as soon as i have established some connections, emacs file saving starts using 100% CPU for about 15 seconds

lmergen15:05:18

i did elp-instrument-package on cider, and it looks like cider-resolve-var has an elapsed time of 13s

lmergen15:05:26

i have 3 repl sessions as we speak

lmergen15:05:15

anyone has an idea what might cause this ?

lmergen15:05:33

i'm running 0.17snapshot, according to cider-version

lmergen15:05:26

it seems like this started getting worse quite recently

bozhidar15:05:10

@lmergen Seems that’s caused by dynamic indentation. You can just disable it.

lmergen15:05:31

does it have to do with agressive-indent-mode ?

bozhidar15:05:39

It’s strange that so many people started reported issues with dynamic font-locking and dynamic indentation lately - we haven’t really changed them in years.

bozhidar15:05:48

> does it have to do with agressive-indent-mode ?

bozhidar15:05:07

The interaction between the two is very painful as the dynamic indent is not as fast as the static one.

bozhidar15:05:23

I’d never use agressive-indent-mode with cider’s dynamic indent.

lmergen15:05:02

let me test without agressive indent

bozhidar15:05:04

That’s one more reminder that we need to cache this info to avoid the excessive calls to cider-resolve-var.

lmergen15:05:16

100% this is the issue

lmergen15:05:24

i disabled the mode and everything's fine

bozhidar15:05:40

(basically you can’t know the indentation for something before you “resolve” it - know what it truly is, and that’s not super fast)

bozhidar15:05:59

Btw, I’ll likely cut the new CIDER release on Monday. If anyone wants to get some last minute fixes in - the weekend is the time to do so. That applies to me as well. 😄

dominicm15:05:34

is that reasonable? or should I retain this behaviour?

dominicm15:05:47

I'm thinking of, at the least, pushing it to the nrepl layer

dominicm15:05:06

the filtering should be taking regexes directly.

bozhidar15:05:53

Yeah, definitely. Some things in orchard look odd just because they simply retained semantics of their middleware usage.

bozhidar16:05:25

I guess you’re referring to the format, right? No idea why this is done on the Clojure side. Maybe to make the clients have to work less.

dominicm16:05:48

yeah, the formatting looks way off to me.

dominicm16:05:15

That was the only thing I could think of, but it seems like it masks complexity, it stops you from using any of the other flags.

bozhidar16:05:22

Yeah, I agree.

bozhidar16:05:39

Likely I didn’t even notice this when the code was originally written.

dominicm22:05:22

I've got a nice set of tests setup for the var/ns filtering, looks great. Passes the apropos tests too, might go back to those and decide about call signatures, but otherwise looking good.

dominicm22:05:47

Oh, there's an apropos test which tests the namespace list, which is gone now. I'll re-implement some of those in terms that make more sense as a whole.

dominicm22:05:57

okay, now the tests are really passing 🙂 I commented out the ordering rules and didn't notice.

dominicm23:05:08

I'm expecting to do a pull request on orchard tomorrow morning, and then start finishing my changes to cider tomorrow, hopefully a pull request in the afternoon.