I'm refactoring the MockFactory into a builder pattern

I'm back.

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

great

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

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

You mean to actually use it?

Sure.

we'll see if it creates new problems :)

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

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

new == coupling == just as hard to test

@Hosch250 yes!

OK.

@Mat'sMug One more question.

sure

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?

what do you mean

Back when I created the Selection property.

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

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

Yep.

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

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

right

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

yes

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

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?

Yes.

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

@Hosch250 hey.

@Rossco hey.

You still having issues with that static method?

Well, sort of.

Can you point me to the code

?

We implemented a wrapper and factory for it.

ah, sounds complicated.

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.

Yeah, extension methods...

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

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

Unit tests will help with that.

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

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

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

screenshot.

please, that is.

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

doesn't work when I click it.

;-D

364 errors in 28 files :(

What did I get into?

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

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

lol

At least you are herding them all into one file.

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).

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.

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

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

59 errors in 26 files.

Crap.

Well, I think it will work.

I just changed = new ValueDeclaration() part.

whatever works :)

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

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?

Declarations and ValuedDeclarations use a class that uses it.

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

ok

It starts in App.cs.

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

awesome

Crap.

It is getting into the inspections now.

its-everywhere

It is like Smallpox.

Do we seriously need it really?

?

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....

it being.. the extension method?

Yes.

The only trouble is ForceFocus then.

But, that would still be the trouble...

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

which part did I miss?

NVM.

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

Might as well finish now, though.

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

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();

where's that

CodeInspectionsToolbar.

OnNavigateCodeIssue().

It is just repeating itself.

that code should be very similar to that of the CodeInspectionsWindow

indeed, looks wrong

Unless it got forgotten again.

very possible

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

(lol, don't do that ;)

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.

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

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"?

@RubberDuck @Mat'sMug Question for you.

@Rossco yeah

hi

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

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

Go To labelName

yes

wait no

GoTo labelname

Go To isn't valid

but the VBE rewrites it

really?

to GoTo labelName

try it.

wow

just ...wow

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

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

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

It builds, and all tests pass.

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

Holy carp @Rossco... Unreal man.

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.

Crap.