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

Additional testing and fixes related to Swagger provider #150

Merged
merged 2 commits into from
Oct 20, 2017

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Oct 20, 2017

The Swagger provider does some interesting things like using one generated type as the base type for other generated types. This causes lookups on the types implied by the generated assembly before generation is complete.

This PR adjusts the translation implementation of the SDK to allow for this, and adds a specific test case for this

@dsyme dsyme merged commit 27232ac into fsprojects:master Oct 20, 2017
let simpleName = Path.GetFileNameWithoutExtension(assemblyFileName)
ProvidedAssembly(AssemblyName(simpleName), assemblyFileName)

new (assemblyName, assemblyFileName) =
Copy link
Member

Choose a reason for hiding this comment

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

Previously, when passing a filename, you didn't need to specify the assemblyname - It seems like this is almost always going to be Path.GetFileNameWithoutExtension off the assemblyFileName - should it just be taken care of for the caller, so they don't need to build the assembly name as well? (That's closer to the old API - you just passed one string for path)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's now a parameterless constructor for ProvidedAssembly https://github.com/fsprojects/FSharp.TypeProviders.SDK/blob/master/src/ProvidedTypes.fsi#L400 that generates a temporary name - is that ok?

Copy link
Member

Choose a reason for hiding this comment

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

I like the parameterless constructor, but I do not like the implementation of Path.GetTempFileName

Creates a uniquely named, zero-byte temporary file on disk and returns the full path of that file.

The GetTempFileName method will raise an IOException if it is used to create more than 65535 files without deleting previous temporary files.

Once, I already wrote the code that calls Path.GetTempFileName too often and did not delete this empty file... This is weird.

Not sure if it is the case... But in theory, TP may crash in looong running VS/IDE instance 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sergey-tihon Yes, you're correct, we are creating empty files, that is wrong

@Thorium
Copy link
Member

Thorium commented Oct 21, 2017

A parameter-less constructor would be nice:

ProvidedConstructor([ProvidedParameter("p", typeof<int>)])

Current workaround:

let empty = fun (_:Expr list) -> <@@ () @@> // <-- some ascii art
ProvidedConstructor([(* ... *)], empty)

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.

4 participants