GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Only use fast path when extension is CSX or none by filipw · Pull Request #980 · scriptcs/scriptcs

Conversation

@filipw

@filipw filipw commented Mar 21, 2015

Copy link
Copy Markdown
Member

Fixes #964 and unblocks 0.14 release

The fast path is only invoked if there is:

  • no explicit module provided AND
  • file extension is "csx" OR no extension (REPL mode)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
filipw added 2 commits March 21, 2015 20:18
@filipw
only use fast path when extension is CSX or none
b8964f4
@filipw
added tests
8814436
adamralph
adamralph reviewed Mar 21, 2015
View reviewed changes
Comment thread src/ScriptCs.Hosting/ModuleLoader.cs Outdated

@adamralph adamralph Mar 21, 2015

Copy link
Copy Markdown
Contributor

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.

Should we make the comparison with ".csx" case insensitive?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@filipw filipw Mar 21, 2015

Copy link
Copy Markdown
Member Author

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.

good point

On 21 March 2015 at 21:48, Adam Ralph [email protected] wrote:

In src/ScriptCs.Hosting/ModuleLoader.cs
#980 (comment):

@@ -69,7 +71,8 @@ public ModuleLoader(IAssemblyResolver resolver, ILog logger, Action<Assembly, Ag
{
if (modulePackagesPaths == null) return;

  •        if (moduleNames.Length == 1 && DefaultCSharpModules.ContainsKey(moduleNames[0])) // only CSharp module needed

  •        // only CSharp module needed - use fast path

  •        if (moduleNames.Length == 1 && DefaultCSharpModules.ContainsKey(moduleNames[0]) && (extension == DefaultCSharpExtension || string.IsNullOrWhiteSpace(extension)))

Should we make the comparison with ".csx" case insensitive?

—
Reply to this email directly or view it on GitHub
https://github.com/scriptcs/scriptcs/pull/980/files#r26896677.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@adamralph

adamralph commented Mar 21, 2015

Copy link
Copy Markdown
Contributor

Whilst we're here, should we also instantiate DefaultCSharpModules as case insensitive? I.e.

internal static readonly Dictionary<string, string> DefaultCSharpModules =
    new Dictionary<string, string>(StringComparer.CurrentCultureIgnoreCase)
    {
        {"roslyn", "ScriptCs.Engine.Roslyn.dll"},
        {"mono", "ScriptCs.Engine.Mono.dll"}
    };
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@adamralph adamralph added the child PR label Mar 21, 2015
adamralph
adamralph reviewed Mar 21, 2015
View reviewed changes
Comment thread src/ScriptCs.Hosting/ModuleLoader.cs Outdated

@adamralph adamralph Mar 21, 2015

Copy link
Copy Markdown
Contributor

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.

This blows up with a NullReferenceException if extension is null (REPL). You'll have to use string.Equals(extension, DefaultCSharpExtension, StringComparison.OrdinalIgnoreCase)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adamralph adamralph Mar 21, 2015

Copy link
Copy Markdown
Contributor

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.

Or you could swap the order of the conditions around the ||, but may as well just call the static on string.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@filipw
ignore case when comparing extension
9e7b063
@filipw
filipw force-pushed the bugfix/964 branch from 4b5b81a to 9e7b063 Compare March 21, 2015 21:50
@adamralph

adamralph commented Mar 21, 2015

Copy link
Copy Markdown
Contributor

The Travis build is still so flaky... IIRC I've seen the problem before. It's a race condition in xunit 1.0. I don't know why it only manifests in Linux though.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@adamralph

adamralph commented Mar 21, 2015

Copy link
Copy Markdown
Contributor

LGTM. The module name casing can be left for now, that's a wider problem which isn't catered for in ScriptServicesBuilder either (and is unlikely to be affecting anyone anyway).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
adamralph added a commit that referenced this pull request Mar 21, 2015
@adamralph
Merge pull request #980 from filipw/bugfix/964
d238444 
Only use fast path when extension is CSX or none
@adamralph
adamralph merged commit d238444 into scriptcs:dev Mar 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

child PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Only scan for modules when -m is specified

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@filipw @adamralph
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.