Fix begin/end replacement in varname expansion (fixes #23)

[phipsgabler]

The trick is to (1) first extract the indices, then replace the begin/end, and (2) make use of the internal Base.replace_ref_(begin_)end_!, which lets you specify the replacement yourself.

The latter seems a bit dubious, but I don't see another chance without a complete reimplementation of Base.replace_ref_begin_end!. Any kind of recursive implementation will have that argument anyway, since you need to care about nested scopes, so it should be kinda stable... I hope.

I'd also like to include a test of the following kind:

julia> vinds(:(x[:, a[10][end], :]))
:(((:, let var"##S#266" = a[10]
              var"##S#266"[(lastindex)(var"##S#266")]
          end, :),))

But those gensyms won't be liked by the doctests. Any idea how to nicely add it?

@torfjelde check it out.

[phipsgabler]

I think it should work now. But there's even more gensyms in the doctests, which it isn't gonna like.

[torfjelde]

Just make them evaluated expression rather than the generated Expr, e.g.

julia> x = [[1.0, 2.0]]; eval(vinds(:(x[1][end])))
((1,), (2,))

[coveralls]

Pull Request Test Coverage Report for Build 1062557433

• 39 of 43 (90.7%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+2.7%) to 85.714%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/varname.jl	39	43	90.7%

Totals
Change from base Build 1028434380:	2.7%
Covered Lines:	78
Relevant Lines:	91

---

💛 - Coveralls

[codecov]

Codecov Report

Merging #25 (41ab5ea) into main (f9fa387) will increase coverage by 2.66%.
The diff coverage is 90.69%.

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   83.05%   85.71%   +2.66%     
==========================================
  Files           1        1              
  Lines          59       91      +32     
==========================================
+ Hits           49       78      +29     
- Misses         10       13       +3     

Impacted Files	Coverage Δ
src/varname.jl	85.71% <90.69%> (+2.66%)	⬆️

---

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 f9fa387...41ab5ea. Read the comment docs.

[torfjelde]

Is this good for a review?

[phipsgabler]

@torfjelde Yes, I think so.

[torfjelde]

Good stuff! I've left a couple of comments, the most important one being that we should probably use Base.maybeview rathe than not.

IMO we should have integration tests for DPPL in AbstractPPL.jl, but for now, could you just run DPPL tests using this branch of APPL to verify that things work? In particular I'm curious about Julia 1.3.

Oh and could you bump the patch-version?

[torfjelde]

Suggested change

	```julia
	```jldoctest

[phipsgabler]

Does that work with a non-exported function?

[phipsgabler]

IMO we should have integration tests for DPPL in AbstractPPL.jl, but for now, could you just run DPPL tests using this branch of APPL to verify that things work? In particular I'm curious about Julia 1.3.

I had the same thought, but Hong recently told me they exist already: https://github.com/TuringLang/AbstractPPL.jl/blob/main/.github/workflows/IntegrationTest.yml. Is that enough?

[torfjelde]

Oh yeah, crap. It's because we've bumped the minor version due to changes in string for VarName. We need to bump compat with DPPL. I'll get on that. Once that's done, the integration tests will run properly.

[phipsgabler]

Closed in favour of #26, which implements a superset of this functionality.