VBA Rubberducking

Mat's Mug

21:07

I'm refactoring the MockFactory into a builder pattern

Hosch250

I'm back.

Duga

[Hosch250/Rubberduck] Hosch250 pushed commit 8224f49d to next: Fix bug.

Mat's Mug

great

Hosch250

@Mat'sMug If you are ready, I can PR.

Mat's Mug

might as well go ahead and refactor the extension method usages, no?

Hosch250

21:14

You mean to actually use it?

Sure.

Mat's Mug

we'll see if it creates new problems :)

Hosch250

@Mat'sMug I'm almost thinking it would be better to new it up.

Either that, or pass the CodePane into Create().

Mat's Mug

new == coupling == just as hard to test

@Hosch250 yes!

Hosch250

OK.

Mat's Mug

21:33

1 hour ago, by Mat's Mug

var wrapper = _factory.Create(someCodePane);
wrapper.SetSelection(...);

Hosch250

@Mat'sMug One more question.

Mat's Mug

sure

Hosch250

GetSelection() was a QualifiedSelection, and SetSelection() took a Selection().

You had me combine those into a single property.

But, now I'm having to create QualifiedSelections just to set a selection that I only use the selection for.

Are you sure I shouldn't split those apart again?

Mat's Mug

what do you mean

Hosch250

Back when I created the Selection property.

Mat's Mug

21:37

at the end of the day you need to call into the VBIDE API

codePane.SetSelection(selection.StartLine, selection.StartColumn, selection.EndLine, selection.EndColumn);

Hosch250

Yep.

GetSelection() returns a QualifiedSelection(): github.com/Hosch250/Rubberduck/blob/…

SetSelection() takes a Selection(): github.com/Hosch250/Rubberduck/blob/…

Mat's Mug

right

Hosch250

When I combined those, I have to pass a QualifiedSelection() into SetSelection, even though I only use the Selection().

Mat's Mug

yes

Hosch250

So, do I just change all the calls to this? codePane.Selection = new QualifiedSelection(new QualifiedModuleName(), selection);

Mat's Mug

21:39

and you can throw an InvalidOperationException if the qualified selection you're getting isn't in the CodePane instance you've wrapped

I'm not following

oh

ideal would be to get/set a Selection, right

and let the caller qualify it as needed

would that work?

Hosch250

Yes.

Mat's Mug

cool

I'm not going to refactor all the existing tests to use the builder, so the MockFactory is staying for now... but I'll put up an issue for it on GitHub at one point.

builder pattern is pretty nifty

Rossco

22:02

@Hosch250 hey.

Hosch250

@Rossco hey.

Rossco

You still having issues with that static method?

Hosch250

Well, sort of.

Rossco

Can you point me to the code

?

Hosch250

We implemented a wrapper and factory for it.

Rossco

22:03

ah, sounds complicated.

Hosch250

github.com/Hosch250/Rubberduck/blob/…

So, right now, we are DI'ing the factory into everything and wrapping all the calls.

Just 56 errors in 25 files left.

Rossco

Yeah, extension methods...

I was going to say if it was an actual static call you could have done something else.

Mat's Mug

@Hosch250 with experience you'll get to refactor code without breaking anything in the process ;-)

Rossco

Unit tests will help with that.

Hosch250

@Mat'sMug I could have, but I deliberately removed the extensions file so it would break.

Rossco

22:06

Its easy to keep it compiling, but making sure all functionality is there is a different story.

Hosch250

Now, R# shows me everywhere I need to inject the factory and wrap it.

Rossco

screenshot.

please, that is.

Hosch250

Just click that red link in the bottom right corner, and R# will cycle through the errors.

Rossco

doesn't work when I click it.

;-D

Hosch250

364 errors in 28 files :(

What did I get into?

Mat's Mug

22:11

@Hosch250 that works.. if you fix 'em all in 1 shot :)

Hosch250

@Mat'sMug Not going to be doing anything else anyway...

Rossco

3

Mat's Mug

lol

Hosch250

593 errors in 27 files :^(

Rossco

At least you are herding them all into one file.

Hosch250

22:19

I just hit that file.

It is the ValuedDeclaration file.

Or rather, the VbaStandardLib file, where they are all declared.

The ValuedDeclarations and Declarations are pains (and those are plurals).

Rossco

BTW, had you considered passing in the static methods as Action<T>/Func<T>?

It does kind of break the semantics but it could work.

Extension methods are a leaky abstraction in the end.

Hosch250

I think it would be just as easy to do this.

Mat's Mug

careful with VbeStandardLib. except the Declarations getter, its members are only ever called via reflection.

Hosch250

59 errors in 26 files.

Crap.

Well, I think it will work.

I just changed = new ValueDeclaration() part.

Mat's Mug

whatever works :)

Hosch250

22:34

Well, it sure would be easier to just new the factory up, but I'm DI'ing it everywhere to make testing easy later.

Mat's Mug

you can't inject it into VbaStandardLib

or are you making it non-static?

not sure why VbaStandardLib would need an instance of the wrapper factory though...

@Hosch250 what's going on?

Hosch250

Declarations and ValuedDeclarations use a class that uses it.

So, I DI'ed it, and it is running everywhere!

Mat's Mug

ok

Hosch250

It starts in App.cs.

I figured you wouldn't want it starting in Extension.cs.

Mat's Mug

awesome

Hosch250

22:43

Crap.

It is getting into the inspections now.

Mat's Mug

its-everywhere

Hosch250

It is like Smallpox.

Do we seriously need it really?

Mat's Mug

?

Hosch250

Well, the extensions are really just a wrapper for the built-in methods that take a Selection.

And the Selection has the startLine... that we need to call the VBE method.

So, we could really just update the call sites to not use it....

Mat's Mug

it being.. the extension method?

Hosch250

22:46

Yes.

The only trouble is ForceFocus then.

But, that would still be the trouble...

Mat's Mug

I thought that was the idea: replace extension method calls with calls to the wrapper

which part did I miss?

Hosch250

NVM.

I was thinking just remove the selection extensions and just update the calls, no wrapper.

Might as well finish now, though.

Mat's Mug

inlining the extension method into call sites would work, yes... but then that's a shit load of repetitive code no?

the wrapper encapsulates a specific functionality that we're using in lots of places

Hosch250

23:03

This looks somewhat suspicious:

                var location = _vbe.FindInstruction(e.QualifiedName, e.Selection);
                location.CodeModule.CodePane.SetSelection(location.Selection);

                var codePane = location.CodeModule.CodePane;
                var selection = location.Selection;
                codePane.SetSelection(selection.StartLine, selection.StartColumn, selection.EndLine, selection.EndColumn);
                codePane.ForceFocus();

Mat's Mug

where's that

Hosch250

CodeInspectionsToolbar.

OnNavigateCodeIssue().

It is just repeating itself.

Mat's Mug

that code should be very similar to that of the CodeInspectionsWindow

indeed, looks wrong

Hosch250

Unless it got forgotten again.

Mat's Mug

very possible

I'm sure we could comment-out the entire toolbar and nobody would even notice

(lol, don't do that ;)

RubberDuck

23:13

I missed a lot, but no @Mat'sMug I've not written that wrapper. So far, I've been able to provide everything the static methods need, but I did have issues with some of the extension methods in the Unit Testing UI layer.

Can't recall exactly what it was now.

Mat's Mug

in any case, that wrapper won't hurt

@RubberDuck while you're here... would you happen to know if CodePane.Name matches the name of the VBComponent it's for, in all instances?

oh nevermind

CodePanes.Parent is VBE

damn that object model is confusing

hey @nhgrif

RubberDuck

23:40

It should, but I can't swear it.

Aren't there some weird edge cases where the name of a module doesn't match the "code name"?

Rossco

@RubberDuck @Mat'sMug Question for you.

Mat's Mug

@Rossco yeah

nhgrif

hi

Mat's Mug

@RubberDuck can't think of any though...

Rossco

I'm working on my VBA grammar and the specification says that the following is valid:

Go To labelName

Mat's Mug

23:43

yes

wait no

GoTo labelname

Go To isn't valid

Rossco

but the VBE rewrites it

Mat's Mug

really?

Rossco

to GoTo labelName

try it.

Mat's Mug

wow

just ...wow

Rossco

There are other similar things in the spec which get rewritten.

Mat's Mug

23:44

so it's possible a VBA IDE implementation wouldn't be doing that

but if we're only ever extending the VBE (which we are), we wouldn't ever hit that

OTOH if you want to use your grammar to, well, make a VBA IDE...

#LearnEveryday

crazy how everytime I think I've seen the bottom of VBA, something new pops up

gotta pick up something to eat, bbl

Rossco

Well there are a couple of things to consider.

1. The VBE is the reference implementation for VBA.

2. If the VBE rewrites something we could assume that all code will be in that form even if the specification allows for a wider set of syntax.

3. Devs will not be used to any of this additional syntax, See Mat's response ;)

I'm going to suggest that I only accept syntax which the VBE accepts without rewriting it.

This will make the grammar simpler and I can't think of any good reason to allow for the additional syntax.

Other examples are =< becomes <= etc

>< becomes <>

EndIf becomes End If

Hosch250

It builds, and all tests pass.

I may have forgotten to assign the field to the ctor DI'ed value a couple times.

RubberDuck

Holy carp @Rossco... Unreal man.

Rossco

Yeah, I wouldn't be concerned about it but the alternative syntaxes are actually in the spec. I wanted your guys thoughts on it.

Just to make sure I haven't missed something.

Which might be a good reason to support them.

Hosch250

Crap.

Transcript for