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

XSD Support #1149

Merged
merged 19 commits into from
Jul 8, 2018
Merged

XSD Support #1149

merged 19 commits into from
Jul 8, 2018

Conversation

giacomociti
Copy link
Contributor

@giacomociti giacomociti commented Apr 10, 2018

The XML Provider is augmented with XSD support as in FSharp.Data.Xsd.
An new parameter Schema is added. When this parameter is used (instead of Sample), an InferedType is derived from the specified XSD.

In order to leverage the existing implementation, the approach (as made evident in the unit tests) is to derive a type essentially equivalent to the one that would be inferred from a significant set of valid samples. The focus is on elements and not on complex types: provided types correspond to element shapes, not to complex types in the schema.

ToDo:

@ovatsus
Copy link

ovatsus commented Apr 10, 2018

Are you going to include this on the other PR?

@giacomociti
Copy link
Contributor Author

Actually I'm a bit confused, guess I need a "bit for dummies" crash course

@giacomociti
Copy link
Contributor Author

I meant git for dummies

@ovatsus ovatsus reopened this Apr 10, 2018
@ovatsus
Copy link

ovatsus commented Apr 10, 2018

Ok, I see that this actually includes the other PR, so let's keep this as the current one, and close the other one

@ovatsus ovatsus changed the title docs for xsd XSD Support Apr 10, 2018
@ovatsus
Copy link

ovatsus commented Apr 10, 2018

Avoid doing rebases because that makes things harder, just commit new commits to your baronfelxsd branch and push, and this PR will be updated

@ovatsus
Copy link

ovatsus commented Apr 10, 2018

There's an xsdprovider branch that was @runefs version, I assume your implementation is an evolution of that right?

@giacomociti
Copy link
Contributor Author

actually my implementation is more of a rewrite, there's little in common with the existing branch

@ovatsus
Copy link

ovatsus commented Apr 10, 2018

What I meant is, functionality wise it's a superset, right?

@ovatsus
Copy link

ovatsus commented Apr 10, 2018

I've merged with master and pushed to your branch

@ovatsus
Copy link

ovatsus commented Apr 10, 2018

Maybe you can reuse the test cases from the xsdprovider branch and integrate them here as well? I merged that branch with master and pushed it so it's easy to look at

@ovatsus
Copy link

ovatsus commented Apr 11, 2018

@giacomociti let me know when you're done with your changes, I'll wait for that before doing a more detailed review. I want to get this into 3.0

@giacomociti
Copy link
Contributor Author

The last commit addresses a little issue (see fsprojects/FSharp.Data.Xsd#15).
There's one more fix I'm going to add and then only improvements for docs and test are left before you can make the review

@giacomociti
Copy link
Contributor Author

the last commit was the last fix from FSharp.Data.Xsd fsprojects/FSharp.Data.Xsd#19
I hope that perf impact of using InferdType as cache key is negligible

@ovatsus
Copy link

ovatsus commented Apr 11, 2018

Yes, perf impact should be negligible, but do make sure to add some tests for those elements with the same name (both signature test, and a test that gets the value at runtime)

@giacomociti
Copy link
Contributor Author

@ovatsus I think we have enough tests now, so you may start the code review (unless you want me to also fix the GetSample issue, but it is not trivial and may take a while). In the meantime I can finish the docs, although I'm having troubles with generate.fsx

@ovatsus
Copy link

ovatsus commented Apr 13, 2018

What error are you getting with generate.fsx?

@giacomociti
Copy link
Contributor Author

I got this on Ubuntu:

/home/giacomo/Documenti/Repos/FSharp.Data/docs/tools/generate.fsx(25,1): error FS0078: Unable to find the file 'Fsharp.Charting.dll' in any of
/usr/lib/mono/4.5
/usr/lib/mono/4.5/Facades
/home/giacomo/Documenti/Repos/FSharp.Data/docs/tools/../../packages/test/FSharp.Charting/lib/net45
/home/giacomo/Documenti/Repos/FSharp.Data/docs/tools
/usr/lib/mono/fsharp/

but on Windows it worked.
Let me know if the docs are ok

@ovatsus
Copy link

ovatsus commented Apr 14, 2018

Oh, I don't think FSharp.Charting works on linux, so that should be fine

@ovatsus
Copy link

ovatsus commented Apr 14, 2018

I'll have a look at removing the GetSample method. The way it was done in CsvProvider doesn't work for you as you're passing the schema as the sample to get the common code to download it

@giacomociti
Copy link
Contributor Author

I've found a way to avoid generating GetSamples by factoring out a couple of functions in the common helper code. Maybe you can figure out a better way

@ovatsus
Copy link

ovatsus commented Apr 14, 2018

Since something similar will be needed eventually for JsonSchema, I refactored a bit the generateType folder to support schemas better, still need to finish a couple of more things, will probably push that tomorrow

@giacomociti
Copy link
Contributor Author

I added a couple of tests: the first one cover nillable elements, which are supported albeit they may seem strange.
The second test is on the generated ctors; for all the simple types supported (except xs:date), we check to produce valid xml (as long as the arguments passed are valid because of course the type system cannot enforce certain xsd constraints like for example regex patterns).
We exclude xs:date because of this issue: the xml gets formatted with the time component, hence it is valid as xs:dateTime but invalid as xs:date.
An option would be to remove xs:date from the (already limited) set of simple types supported, falling back to string. On the other hand this would break the nice property of deriving the same InferedType that get infered, without a schema, from valid samples.

…aronfelxsd

# Conflicts:
#	tests/FSharp.Data.Tests/XmlProvider.fs
@ovatsus ovatsus merged commit f43e625 into fsprojects:master Jul 8, 2018
@ovatsus
Copy link

ovatsus commented Jul 8, 2018

This is now released as part of 3.0.0-beta4. Sorry for taking so long

@giacomociti
Copy link
Contributor Author

Great! Thanks @ovatsus

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.

2 participants