Skip to content
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

Rename Source metadata property, clean up resolution logic (#351) #352

Merged
merged 1 commit into from
May 17, 2016

Conversation

mathewc
Copy link
Member

@mathewc mathewc commented May 14, 2016

Addresses bug# #351

Also improving the error message we give when we can't determine the primary script file. As David indicated in the bug, Kudu is using this exact same logic. The plan is to copy this updated function to Kudu as well, and release these updates together.

@davidebbo @ahmelsayed

// First see if there is an explicit primary file indicated
// in config. If so use that.
string functionPrimary = null;
string scriptFileName = (string)functionConfig["scriptFile"];
Copy link
Member Author

@mathewc mathewc May 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property used to be called "source". We're renaming this to "scriptFile". It's an undocumented property at this point, so we'll be safe in making the breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"scriptFile", not "sourceFile" :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep :)

scriptFileName = Path.GetFileNameWithoutExtension(scriptFileName);

functionPrimary = functionFiles.FirstOrDefault(p =>
string.Compare(Path.GetFileNameWithoutExtension(p), scriptFileName, StringComparison.OrdinalIgnoreCase) == 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extension logic doesn't feel quite right. Say the file is named foo.bar.js. It sounds like scriptFile can be either set to foo.bar or foo.bar.js? But if it's just foo.bar, GetFileNameWithoutExtension will end up with just foo. Also, you could end up finding a file that has the same name without extension, but a mismatched extension.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, to avoid all of this, I can just make source file have to be the entire file name. Perhaps that is cleaner and easier to explain.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's probably best. Allowing either form is just asking for trouble, with too little gain to be worthwhile :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants