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

Code review #1

Open
Planeshifter opened this issue Dec 31, 2015 · 1 comment
Open

Code review #1

Planeshifter opened this issue Dec 31, 2015 · 1 comment
Assignees

Comments

@Planeshifter
Copy link
Contributor

Open questions:

  • right now, the synchronous version loads the entire file into memory in order to create the data frame. Should we change this?
  • in contrast to some other modules, I chose to make the synchronous version of the function the main export and delegated the asynchronous one to a property given the main use case being the analysis of data in a REPL environment.
  • the analogous function in R provides parameters to specify the row- and column names. Should we offer something similar?

Any other suggestions?

@kgryte
Copy link

kgryte commented Dec 31, 2015

  • For the synchronous version, you don't really have a choice. You have to load the entire file (i.e., do some synchronous file system operation).
  • I would probably still make the async version the main export. For a REPL, e.g., a browser-environment with virtual files, we can, under-the-hood, make the default behavior sync. Meaning, we will have some control over which method is exposed as "the" interface.
  • Specifying row and column names could be left to df methods. Won't really know until df is brought up to speed.

We will need to prioritize how soon df is refactored and integrated into the roadmap. Modules such as this one are useful and will be desired once a REPL env is provided.

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

No branches or pull requests

2 participants