-
Notifications
You must be signed in to change notification settings - Fork 25
Access instructor directly from endpoint #669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: future
Are you sure you want to change the base?
Conversation
bb1ad6c
to
f5f3510
Compare
f5f3510
to
c529d73
Compare
c529d73
to
66b2da7
Compare
Still need to fix the tests. |
Nice – definitely one of the most unnecessary modules we've ever had 👍 Could we also harmonize the Namespaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about bringing back a WorkerSupport
as a resource which is a wrapper around all VisualInsturctors
- this could also do the content-replacement
public void AddInstruction(string identifier, InstructionModel instruction) | ||
{ | ||
_workerSupport.AddInstruction(identifier, Converter.FromModel(instruction)); | ||
} | ||
var instructor = _resourceMgmt.GetResource<IVisualInstructionSource>(identifier); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the idea behind adding instructions from the endpoint?
var content = item.Content; | ||
var preview = item.Preview; | ||
|
||
// Apply content pattern | ||
if (!string.IsNullOrEmpty(_config.ContentReplacement)) | ||
{ | ||
// Fill content with content regex | ||
if (_contentRegex != null && !string.IsNullOrEmpty(item.Content)) | ||
item.Content = _contentRegex.Replace(content, _config.ContentReplacement); | ||
// If replacement was configured but no regex, we apply the preview regex | ||
else if (_previewRegex != null && !string.IsNullOrEmpty(item.Preview)) | ||
item.Content = _previewRegex.Replace(preview, _config.ContentReplacement); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have dropped the support for replacing regex - I don't really know, but was it an unused feature? I remember features like Instruction.Content = "<guid>|Master"
which was then replaced by http://server/media/...
- maybe we could move that to the VisualInstuctor
itself?
The module was introduced as part of the business and licensing model. Dropping it actually simplifies the endpoint.