automatic peco install on julia>=1.3

[rapus95]

trafficstars

This was the shortest change I had been able to come up with. It now downloads in any case but uses the systems peco if available. (Otherwise it uses the artifact)

Fixes #6 for Julia>=1.3

Edit: I even managed to increase coverage 🤣

EDIT2: Seems to be far from idiomatic usage of the new artifacts & jll. Thus, do not merge yet! Eventually this needs a bigger rewrite.

[tkf]

Thanks for the PR.

BTW, I'm OK with dropping julia 0.7 if that's the blocker.

[rapus95]

It's not the 0.7 but my current usage of peco(identity) as I was told in the slack thread. I'll reopen it but will try to redesign it to match how it should be used.

For that we need a new interactive_matcher design.

[codecov-io]

Codecov Report

Merging #10 into master will increase coverage by 0.57%. The diff coverage is 93.75%.

@@           Coverage Diff           @@
##           master   #10      +/-   ##
=======================================
+ Coverage   73.42%   74%   +0.57%     
=======================================
  Files           4     4              
  Lines         350   350              
=======================================
+ Hits          257   259       +2     
+ Misses         93    91       -2

Impacted Files	Coverage Δ
src/InteractiveCodeSearch.jl	89.5% <93.75%> (+1.23%)	:arrow_up:

---

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 8c442ab...c43e21a. Read the comment docs.

[rapus95]

Now it might be ready to go, but I found that the visual history of what's been written previously just vanishes after the query.

EDIT: Fixed tests

[rapus95]

After force pushing like 5 times, I MIGHT managed to fix everything.

Who expects that tests are written with setfield! 😁

EDIT: cat is not a valid executable on windows -> Tests on Windows will always error

[rapus95]

Now 0.7 results in unsatisfiable constraints because peco_jll has constrained julia to be >=1. So maybe we should drop it now. But tbh why would anyone stay on 0.7?

[rapus95]

Btw why there are no CI jobs for JL 1.2 and 1.3? (And soon 1.4)

[tkf]

For that we need a new interactive_matcher design.

Did you see https://github.com/tkf/InteractiveCodeSearch.jl/issues/6#issuecomment-574494692? What do you think about implementing Option 2 here?

Now 0.7 results in unsatisfiable constraints because peco_jll has constrained julia to be >=1.

Sounds good. Please bump the minor version of InteractiveCodeSearch.jl in Project.toml as well when doing this.

Btw why there are no CI jobs for JL 1.2 and 1.3? (And soon 1.4)

Good catch. I forgot to add them. Let's add them here as 1.3 is necessary for _jll test.

[rapus95]

Did you see #6 (comment)? What do you think about implementing Option 2 here?

I didn't see it. Regarding CONFIG.interactive_matcher = we also could customize getproperty, though, as you neither exported CONFIG nor CONFIG.interactive_matcher it can be questioned wether that's a breaking change since we can consider that stuff implementation detail which can be subject to change (And changing for a new minor version of Julia sounds legit to me). Since accessing fields usually is bad style in Julia anyway I'm curious why you want to retain that style rather than switching to some setmatcher! function.

[tkf]

as you neither exported CONFIG nor CONFIG.interactive_matcher it can be questioned wether that's a breaking change since we can consider that stuff implementation detail

CONFIG is a public API because it is documented: https://github.com/tkf/InteractiveCodeSearch.jl#interactivecodesearchconfig

Since accessing fields usually is bad style in Julia anyway

I think it's a totally valid style as long as it is documented (i.e., a public API). See, e.g., DataFrames.jl and Tables.jl. Also, this CONFIG.<property_name> = <value> style is used elsewhere too:

• Cthulhu.jl: https://github.com/JuliaDebug/Cthulhu.jl/blob/f6d225a7ac192d57d9e526eb053e4e86161d9bf4/src/Cthulhu.jl#L18 (though it's not mentioned in README)
• ElectronDisplay.jl: https://github.com/queryverse/ElectronDisplay.jl#configuration

OK, full disclosure, these are all initially added by me. But I didn't have major backlash so I'm assuming that it's a valid interface.

[rapus95]

So what do you think about the get/setproperty function? Btw, removing ::Cmd would be breaking then aswell because if for whatever reason someone decides to read that attribute he won't necessarily get a cmd object anymore. (especially if reading the default we set.) if only writing to it then setproperty! might be able to solve everything.

[tkf]

So what do you think about the get/setproperty function?

Do you mean to do the conversion via setproperty!? I think it's a possible solution. But I'm reluctant to do this as it breaks the property

CONFIG.interactive_matcher = value
@assert CONFIG.interactive_matcher == value

It's OK to break it sometimes (especially in the class-based OOP) but I'd like to avoid it if possible.

whatever reason someone decides to read that attribute he won't necessarily get a cmd object anymore

That's a fair point but I only document it for setting things. Also, overloading setproperty! breaks the "read API" more (as it breaks the above property).

[rapus95]

Either way will be breaking, so how about doing a fair cleanup (and bumping the major version)? Aka condensing the code we need and adding an explicit interface via methods

[tkf]

As I said, I don't think CONFIG.<property_name> = <value> is a bad interface. In fact I think it has nice properties for configuration system:

• Consistent get/set method. No need for defining separate methods get<property_name> and set<property_name>! where they are paired only by naming convention.
• Configurable properties are documented in a single place (i.e., ?InteractiveCodeSearch.CONFIG works).
• Configurable properties can easily be TAB-completed.

[rapus95]

While it's true that CONFIG seems to be a good place, having interactive_matcher untyped doesn't seem so. Tab completion doesn't help if you can't infer which type you need to provide. A setmatcher! would overcome that by providing the necessary info even if there are no docs. 😄 Either way, to me that object-y design feels somewhat uncomfortable but after adding peco via artifacts I don't intend to ever use that interface anyway 😁

[tkf]

A setmatcher! would overcome that by providing the necessary info even if there are no docs.

Unfortunately, it doesn't. In Julia, every type is potentially callable. So, for hypothetical API setmatcher!, the signature we must use is setmatcher!(callable::Any). No type checking is possible at set-time and we can't use type annotation as "documentation."