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

Table should provide more read options #29

Closed
OriHoch opened this issue Jul 13, 2017 · 6 comments
Closed

Table should provide more read options #29

OriHoch opened this issue Jul 13, 2017 · 6 comments

Comments

@OriHoch
Copy link
Collaborator

OriHoch commented Jul 13, 2017

feature requests from @roll (See original request in #25)

Table class should provide more read options:

  • option to get uncast data
    • e.g. table.read(cast=false) // list of strings
    • It allow to work with malformed data sources and validate it e.g. filed-based with custom error handling.
  • I think we need provide documentation how e.g. read(limit=10) could be achieved
  • Based on readme only keyed rows are emitted. Python and JavaScript also support:
    • default rows ['value1', 'value2', ...] - esp. useful with malformed data and cast=false (because header-values map doesn't work in this case)
    • extended rows [1, ['header1', 'header2'], ['value1, value2']] - to get row number
@OriHoch
Copy link
Collaborator Author

OriHoch commented Jul 13, 2017

Because PHP doesn't have named parameters, and also to keep Table class small and simple, I'm thinking of an interface like this:

$table->reader()->cast(false)->read();
$table->reader()->keyed(false)->limit(5)->read()

Could also provide an iter() method that instead of returning an array will return an iterator

The reader method will return a TableReader class that will handle all that logic

@OriHoch
Copy link
Collaborator Author

OriHoch commented Jul 13, 2017

all the functionality needed to implemented the TableReader is already available in the Table class

  • cast(false) can get the data directly from the datasource
  • keyed / limit /extended can do normal iteration over the Table and manipulate the values it returns / when to stop iterating

@roll
Copy link
Member

roll commented Jul 13, 2017

Because PHP doesn't have named parameters, and also to keep Table class small and simple, I'm thinking of an interface like this

Does PHP support JavaScript/Ruby pattern like?

function(arg, options)

To have something like:

$table.iter(["cast" => true])
$table.read(["keyed" => true])

@OriHoch
Copy link
Collaborator Author

OriHoch commented Jul 13, 2017

yes, but it has too many disadvantage IMO, mainly:

  • prone to human error (e.g. misspell of an attribute)
  • hard to use from REPL / IDE - no autocompletion

both points can be solved by:

  • validation in the code
  • keeping documentation updated

both require more work from developers of the library, easy to forget to update documentation / validation

That's why I think the solution I proposed is better (I would recommend it for JS as well, don't know about Ruby).

There is another option for using parameters object, but that's very cumbersome, especially in PHP

@roll
Copy link
Member

roll commented Jul 13, 2017

That's good points. Just a few thoughts from the whole ecosystem perspective:

  • I'm a little bit afraid of some over-engineering on this stage of work for any implementation (chainable reader API?)
  • and also really like to have maximum portability of users knowledge across ecosystem. The most tutorials and examples for Frictionless Data is(will) be written for Python/JavaScript. This two implementations are now almost clones (Python uses keyword arguments and for JavaScript pattern matching for keyword arguments is core concept nowadays so it's pretty OK to use). And it's good that's it's so close. Ruby is really close too. So it will be great to have as much as possible skills convertible to work with PHP implementation too. So I think it could be more important than autocompletion etc. Other question if it's an anti-pattern for PHP. But if not still think is a good enough for now (I suppose could be much easier to implement than reader thing).

@roll roll changed the title Table: Should provide more read options Table should provide more read options Nov 10, 2017
@roll roll added this to the Version-1 milestone Nov 10, 2017
@OriHoch
Copy link
Collaborator Author

OriHoch commented Nov 30, 2017

done in f97fd9d

@OriHoch OriHoch closed this as completed Nov 30, 2017
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