Adding ScriptPath to Env #1192
Adding ScriptPath to Env #1192
Conversation
|
Added |
|
Here is a screenshot of both new properties being used.
|
| } | ||
|
|
||
| public string ScriptPath { get; set; } | ||
| public IList<string> LoadedScripts { get; private set; } |
There was a problem hiding this comment.
I think maybe this can even be HashSet<string> since there is no need to show the same script twice
There was a problem hiding this comment.
I don't think we should use Hashset as it does not guarantee ordering. I want the order to be the load order.
|
maybe we could add current script to |
|
Great minds think alike, I thought about that right after :-)
…On Mon, Jan 9, 2017 at 1:27 AM Filip W ***@***.***> wrote:
maybe we could add current script to FilePreProcessorResult as a
standalone property, then we could avoid relying on
result.LoadedScripts.Skip(1) (first == current script) as I have a
feeling this might become a problem in the future
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1192 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAInRCKrZIdRGZJJGQr2oc4VKhdHZL8Eks5rQf12gaJpZM4Ld-0n>
.
|
|
What makes you think it would be a problem? The code is recursive and the
first thing it hits is the makn file? I thought of doing it because it felt
a little hacky, but not that it would break later.
…On Mon, Jan 9, 2017 at 6:36 AM Glenn Block ***@***.***> wrote:
Great minds think alike, I thought about that right after :-)
On Mon, Jan 9, 2017 at 1:27 AM Filip W ***@***.***> wrote:
maybe we could add current script to FilePreProcessorResult as a
standalone property, then we could avoid relying on
result.LoadedScripts.Skip(1) (first == current script) as I have a
feeling this might become a problem in the future
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1192 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAInRCKrZIdRGZJJGQr2oc4VKhdHZL8Eks5rQf12gaJpZM4Ld-0n>
.
|
|
Man Travis STILL causes us problems.....this is annoying |
…/ FilePreProcessorResult and removing skip logic
|
@filipw I did a refactoring so skip is no longer needed / I made everything first class in the FilePreProcessor. This provides other benefits so that there is no magic logic needed upstream. |
|
cool - LGTM |
|
@filipw I said "DON'T MERGE" did you read the top? :-) I wanted to add unit tests. But I can add those separately now I guess. |
|
haha I thought that was regarding the 1st commit, and that subsequent commits negated it 😄 |
|
:-) Next time I'll put checkboxes. with a title like "Filip, don't commit until these are done:" :p |
2 participants
DO NOT MERGE YET
Fix for #225
Creating this PR so others can review the approach.
This adds a new ScriptInfo class that is set with the path of the file that is executed. Ultimately the path is exposed off of the ambient
Envproperty. The approach minimizes signature changes as it depends on ctor injection to get the ScriptInfo class passed around.@scriptcs/core @rossipedia appreciate your thoughts.
Yes this issue is 2 1/2 years old.....