Skip to content

only support rows(): AsyncRow[]#36

Merged
severo merged 2 commits intomasterfrom
only-asyncrows
Feb 6, 2025
Merged

only support rows(): AsyncRow[]#36
severo merged 2 commits intomasterfrom
only-asyncrows

Conversation

@severo
Copy link
Copy Markdown
Contributor

@severo severo commented Feb 6, 2025

This PR is a breaking change in the dataframe's rows method, since it must return AsyncRow[], instead of supporting two types: AsyncRow[] | Promise<Row[]>.

Also, I separate the index from the values to avoid collisions and make the code simpler:

type Cells = Record<string, any>

/**
 * A row where each cell is a resolved value.
 */
export interface Row {
  cells: Cells
  index: number
}

@severo severo changed the title only support rows(): AsyncRow[] + require 'index' + move values to 'c… only support rows(): AsyncRow[] Feb 6, 2025
@severo
Copy link
Copy Markdown
Contributor Author

severo commented Feb 6, 2025

wdyt @platypii?

The internal code is a bit simpler, I think. But the interface to pass the data is a bit more complex (see the changes in the tests). So... not sure

I feel like wrapPromise and asyncRows are still two ways to do the same and we might simplify this too.

Copy link
Copy Markdown
Contributor

@platypii platypii left a comment

Choose a reason for hiding this comment

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

I like this direction. I think that having the __index__ in the same object as the columns was making the code really awkward.

The dataframe interface is okay to be a little complicated especially since we have helpers like asyncRows which can help. There might be some other helpers which could make it easier for a naive implementation (so that down stream users can just have resolved objects and not have to understand asyncrows). But also... no one besides us is using hightable yet, and we should keep evolving it to suit our needs.

@platypii
Copy link
Copy Markdown
Contributor

platypii commented Feb 6, 2025

Should update the README too

@severo
Copy link
Copy Markdown
Contributor Author

severo commented Feb 6, 2025

OK! I'll update the README and merge.

@severo severo merged commit bb6da74 into master Feb 6, 2025
@severo severo deleted the only-asyncrows branch February 6, 2025 21:27
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