this is a nice PR, thanks for fixing this.

One small thing, I noticed that you always initialize the List<Type> for line processors in the overrides dictionary - imho this is unnecessary - you can just initialize it in the TConfig LineProcessor<T>(), the first time some processor will be added. Similarly, no need to throw inside RegisterLineProcessors if line processors are missing from the overrides - then you'd just use the 3 default ones.

Thanks for the feedback. I went back and forth on where the list should be initialized. I'll make the changes tonight and resend.

@filipw, updates committed. Let me know if you have any other suggestions. Thanks again!

Ryan, can I build from https://github.com/ryanrousseau/scriptcs to test?

@cmisztur yep, the changes are in the development branch. Thanks

@ryanrousseau -- works.

Per usage in line 35 I would recommend making DirectiveLineProcessor.DirectiveString property protected instead of private. Unless I'm missing something.

-c

HI @ryanrousseau

Thanks for the work here! Up until now we've had an agreement to not use "#" to introduce custom directives as we wanted to stick to directives understood by the Roslyn compiler. The current agreement was to use "//#" for custom as it's a comment.

I am leaning toward changing my opinion on this, especially based on the fact that we're now supporting the Mono engine which also doesn't understand directives like #load, though it will via scriptcs as we implement that an #r.

I'd be interested to hear what @filipw and @jrusbatch think on this in particular as we originally agreed on this.

@cmisztur you can call the Matches method there, but you shouldn't have to because the protected ProcessLine method is only called when the checks in the public ProcessLine method pass.

@glennblock sounds good. I'll watch for any comments on whether to make that change or not.

If it's decided to stick with //# for custom directives, would that be a guideline or enforced by scriptcs? Enforcing it on classes that implement IDirectiveLineProcessor wouldn't be too hard, but any class that implements ILineProcessor could be written to handle a # directive and may be harder to check.

Sorry I dropped off the earth for a bit, but I was just checking in to see if there was any other feedback or anything else I need to do for this PR?

We decided to drop the restriction :-)

@scriptcs-core any thoughts?

I'm in agreement that modules should be allowed to add # directives freely.

What will be the order of precedence? If a module defines a directive that scriptcs already defines will the module win? I guess that is in line with the purpose of modules: overriding out of the box behaviour.

Looks good to me. @ryanrousseau want to rebase this?

Yeah Adam, if a module wants to override that's fine. It is opt-in for a
user to install a module.

On Monday, May 19, 2014, Kristian Hellang notifications@github.com wrote:

Looks good to me. @ryanrousseau https://github.com/ryanrousseau want to
rebase this?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/632#issuecomment-43549443
.

@khellang so this is probably going to be a dumb question because my git-fu is lacking (I use TFS daily and only have used git in simple scenarios so far). What do I need to rebase: squash my commits down to one, rebase the origin onto my fork (vs merging), both, or something else? Let me know and I'll figure it out.

@khellang I took a guess and think I did the right thing as far as the rebase goes. If not let me know and I'll fix it. It builds on my fork and all tests pass - hoping that the TC build will pick up these changes.

👍

On Tuesday, May 20, 2014, Kristian Hellang notifications@github.com wrote:

[image: ]

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/632#issuecomment-43596253
.

@ryanrousseau something weird happened with that last commit. it looks like you made the change on your old pre-rebased branch then merged it to your rebased branch? Perhaps you did that latest commit on another machine before pulling your rebased dev branch from your fork? Anyway, don't worry about it. If we decide we care about the aesthetics of the history we will cherry pick the commits ourselves.

I'm still mid-way through reviewing your changes (I've never looked at the PR before) but I think we're not far off now 😄

@adamralph that is weird. I was using the same machine and rebased branch, but I did use the Git for Windows client to do the sync to github - so maybe that caused it? I'll watch out for that in the future.

linking to #629 and #630

@ryanrousseau I don't think changing the builder / config to have an Overrides prop instead of implementing the interface is an option, it will cause too much breakage to existing modular authors and hosts.

"Is there every a time where would need multiple instances of a set of overrides? Could ServiceOverrides potentially become a singleton instead of being passed between the different classes that use it?"

Singleton as in process-wide?

That I would see as a problem as in a web hosted scenario you could have multiple script builders, etc in the same process.

@glennblock

"I don't think changing the builder / config to have an Overrides prop instead of implementing the interface is an option, it will cause too much breakage to existing modular authors and hosts."

Yeah - I was thinking they might be able to keep the interface and pass through to the methods on the property, but that is probably too much duplication. Could get the same result by creating a new class to wrap the dictionary and the processor list and exposing / using that instead of the dictionary directly?

"Singleton as in process-wide?

That I would see as a problem as in a web hosted scenario you could have multiple script builders, etc in the same process."

Yeah, that probably wouldn't work then.

It seems like this PR has uncovered part of the API which really does require some more thought (thank you @ryanrousseau!). I suggest we consider the design carefully and come up with something everyone is happy with before continuing.

Personally I don't feel well qualified to give an opinion on the general design of the configuration/overrides/services/registration aspect of the hosting layer right now. I probably need to take a step back and get the structure clear in my head before offering anything more.

Ha, well, I aim to please. I was hoping to be clearing some work off the board instead of adding more to it though 😄

@glennblock @khellang what do you want to do in the mean time - release a quick fix to get line processors working (this fix or another) or wait until the design is final? If it's the latter, I'll close this PR and look at some other issues to work on.

What would be your quick fix proposal? I definitely prefer NOT delaying
this further and really appreciate your patience.

On Thursday, May 22, 2014, Ryan Rousseau notifications@github.com wrote:

Ha, well, I aim to please. I was hoping to be clearing some work off the
board instead of adding more to it though [image: 😄]

@glennblock https://github.com/glennblock @khellanghttps://github.com/khellangwhat do you want to do in the mean time - release a quick fix to get line
processors working (this fix or another) or wait until the design is final?
If it's the latter, I'll close this PR and look at some other issues to
work on.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/632#issuecomment-43881648
.

@glennblock the current changes in the PR could potentially be considered the quick fix since it's done and working, but it does introduce some of the concerns @adamralph mentioned and would potentially be undone in the future. The other fix that @khellang and I discussed briefly was just passing the existing list of line processors to the different objects through their constructors like how the overrides dictionary is passed now. I think that should work too. I'm okay with doing either, or neither if you rather keep it the way it is until the details are figured out.

No worries at all as far as delay or anything goes. I enjoy using scriptcs and appreciate the opportunity to contribute to it.

I am good with either approach as long as existing scenarios are not
broken. @khellang, which do you prefer?

On Thu, May 22, 2014 at 7:59 PM, Ryan Rousseau notifications@github.comwrote:

@glennblock https://github.com/glennblock the current changes in the PR
could potentially be considered the quick fix since it's done and working,
but it does introduce some of the concerns @adamralphhttps://github.com/adamralphmentioned and would potentially be undone in the future. The other fix that
@khellang https://github.com/khellang and I discussed briefly was just
passing the existing list of line processors to the different objects
through their constructors like how the overrides dictionary is passed now.
I think that should work too. I'm okay with doing either, or neither if you
rather keep it the way it is until the details are figured out.

No worries at all as far as delay or anything goes. I enjoy using scriptcs
and appreciate the opportunity to contribute to it.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/632#issuecomment-43922372
.

@ryanrousseau @glennblock @khellang @adamralph I would say to bring it in as-is and not continue to worry too much over the fact that you can register any type as ILineProcessor etc.

This part of the API is intended to be used in hosting so typically by someone who knows what he's doing in the first place.

As mentioned, it can be modified in the future should it become a bigger issue, but regardless, it is a huge step forward for us, because in the current solution line processors do not work at all so I really think we should get it in ASAP.

I agree that this is a hugely valuable change and it will work. My only worry is that we'll have to make a breaking change later after giving the API some more thought but that's often the natural path of progression and we shouldn't let it hold us back, so I also say let's get it in.

Having said that, I do think that the points I mention need addressing - #632 (diff) and #632 (comment).

Either @ryanrousseau can do this or we can merge and send a follow up (which I'm happy to take on).

Thanks @ryanrousseau! Sorry it got dragged out.

@scriptcs/core I'll send a follow up PR shortly to address the minor points.

excellent - thanks to everyone involved!

@adamralph 👍 I can make those changes

Nevermind, you closed as I was typing. Thanks very much! Don't worry at all. They're valid points and need to be addressed. I was just trying to stay on top of this PR without being too troublesome since I let it sit for a month without following up on it.

I do have one question. Does ServiceOverrides.Overrides need to be public? I don't know the history of it or if it was meant to be exposed. In the solution, the only usages I've found for it outside of ServiceOverrides is in ScriptServicesBuilder.

Could this

_scriptEngineType = (Type)Overrides[typeof(IScriptEngine)];

be replaced with something like this

_scriptEngineType = ScriptEngine();

ScriptEngine() being defined on ServiceOverrides as

    public Type ScriptEngine()
    {
        return (Type)Overrides[typeof(IScriptEngine)];
    }

That may not be the best way to extend the API for ServiceOverrides, but with something like that and Overrides being private/protected, could solve a few of the problems?

Since this is closed now - do you want to open a new issue to continue this discussion?

And also what @filipw said - thanks to everyone else for their help with this.

@ryanrousseau yeah, if you could open a new issue that would be great, and move your last comment over to it, thanks.

BTW - you've been added to https://github.com/scriptcs/scriptcs/wiki/Contributors 😃

@adamralph will do and thanks for the add!