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

Consume the sample data directly by default #12

Closed
ovatsus opened this issue Jan 15, 2013 · 13 comments
Closed

Consume the sample data directly by default #12

ovatsus opened this issue Jan 15, 2013 · 13 comments

Comments

@ovatsus
Copy link

ovatsus commented Jan 15, 2013

The FSharpx type providers for json and xml start with the data already loaded, which is handy for scripting scenarios and demos.
In FSharp.Data we always have to do a .Load or .Parse after defining the type. By default it could load the sample data, and we would only need to call .Load or .Parse if we wanted to override it. You could disable that behavior by passing a LoadSampleData=false

@tpetricek
Copy link
Member

I agree this should be added. I think generating a constructor which loads the sample data (or loads the file specified as a static parameter) would be the best approach. Then you could write:

let csv = new CsvProvider<"file.csv">()

without even defining a new type.

@tpetricek
Copy link
Member

If you (or someone else) wants to add this, then please also update the samples (samples/*.fsx) because they could be nicely simplified if we had this feature.

@ovatsus
Copy link
Author

ovatsus commented Jan 24, 2013

That constructor aproach works fine for CSV, but for JSON and XML it doesn't apply. But instead we can make the parameter of the Load/Parse methods optional, so we that when we don't pass it, it reuses the sample. I'll submit a patch soon

@tpetricek
Copy link
Member

Nice work!

The fact that it now uses different style for CSV than for JSON/XML is a bit unfortunate. What is the reason why using the constructor approach does not work for these two?

Also, I noticed the JSON provider tries to guess whether the string is a JSON using:

if firstChar = '{' || firstChar = '[' then

In principle, I think that true is also valid JSON. It might not be a problem (often) but if we want to do something more clever than just trying to load the file, it would probably be easier to check if the string is a well-formed URL or path.

@tpetricek
Copy link
Member

I think we should aim to have the following interface for all the tree options:

  • static Parase method to parse a string input
  • static Load method to parse a file or URL
  • constructor to parse or load the input according to static parameters

If the constructor takes a string, then it is not clear what is that supposed to mean (it is not too bad, because there is probably a tool tip, but I think having a clear API is better).

@ovatsus
Copy link
Author

ovatsus commented Jan 24, 2013

While the Parse and Load methods of CSV return an instance of CsvFile, on XML/JSON the return type is an instance of a root under the domain types, so we can't just use the constructor.

We have Load and Parse methods for all, and while for CSV we have the constructor, for XML/JSON, the Load or Parse method (according to if we provided a file or inline data) can be called without parameters, so effectivly the static parameters become the default runtime parameters.

I'd also like to make the 3 providers as similar as possible, but they are very different, CSV is a matrix, JSON/XML are trees, so there's more stuff that's probably going to become different between XML/JSON and CSV, like the write api, for example

@ovatsus
Copy link
Author

ovatsus commented Jan 24, 2013

About the guessing in the json provider, I'll fix that, I did it to avoid having a lot of exceptions being thrown while debugging, but unlike the xml is not 100% reliable so it needs to have a fallback

@ovatsus
Copy link
Author

ovatsus commented Jan 24, 2013

Actually, the code is already right, if there's { or [, it knows for sure is not a filename, otherwise it still tries both

@tpetricek
Copy link
Member

Ah, I see.

Okay, let's leave it like this for now, but I would like to avoid this mismatch somehow (but it definitely needs more thinking than I thought :-)).

@ovatsus
Copy link
Author

ovatsus commented Jan 24, 2013

Just consider this a first draft, we'll probably still change the api a couple of times until we're happy.
I think all the providers still need a lot of work until they mature, every time I try them on real data I find another thing I want to change :)

@tpetricek
Copy link
Member

Sure, I'm just thinking aloud here :-) Thanks for all the hard work on improving the providers as well as on the portable version, you've made some excellent progress here!

@ovatsus
Copy link
Author

ovatsus commented Jan 27, 2013

While porting the FSharpx unit tests, I come to the conclusion that .Load() and .Parse() to get the default data is weird, trying something else

@ovatsus
Copy link
Author

ovatsus commented Feb 7, 2013

Are you ok with the current .GetSample() solution for Xml and Json?

@ovatsus ovatsus closed this as completed Feb 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants