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

feat: add drag-and-drop for xlsx-format #2577

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

fnchooft
Copy link
Contributor

@fnchooft fnchooft commented Apr 23, 2024

Dragging an xlsx-file onto a livebook should load the code-snipplet.

In order for the DataTable to load the code assumes that a sheet called "Sheet 1" exists and that the first row contains column-labels.

There are still some corner-cases which have to do with loading data from Excel in the general sense.
The library https://github.com/xavier/xlsx_reader from https://github.com/xavier was used in the addition.

Hopefully some users will find the addition useful.

@CLAassistant
Copy link

CLAassistant commented Apr 23, 2024

CLA assistant check
All committers have signed the CLA.

Dragging an xlsx-file onto a livebook should load the
code-snipplet.

In order for the DataTable to load the code assumes that
the first row contains column-labels.
@fnchooft
Copy link
Contributor Author

Please find a xlsx-file attached - which can be used to test the functionality.
livebook.xlsx

Kind regards,

Fabian

Copy link

github-actions bot commented Apr 25, 2024

Uffizzi Preview deployment-50811 was deleted.

@josevalim josevalim merged commit c458a06 into livebook-dev:main Apr 25, 2024
6 of 7 checks passed
@josevalim
Copy link
Contributor

💚 💙 💜 💛 ❤️

@fnchooft
Copy link
Contributor Author

Oh my - i believe "mix format" needs a run?

:) Thanks for the support - please let me know if you want me to do this.

Kind regards,
Fabian

@jonatanklosko
Copy link
Member

No worries, fixed on main :)

tabs =
for sheet <- XlsxReader.sheet_names(package) do
# Assume the first row contains column names
{:ok, [header | rows]} = XlsxReader.sheet(package, sheet)
Copy link
Contributor

Choose a reason for hiding this comment

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

I've just tested with some real sheets, and got a problem here in a scenario that I think it's very common:

the file has several sheets with values, but one of the sheets is empty and it fails matching in this line.

how about doing something like below?

tabs =
  for sheet <- XlsxReader.sheet_names(package),
      {:ok, rows} = XlsxReader.sheet(package, sheet),
      length(rows) > 1 do
    [header | rows] = rows

    maps =
      Enum.map(rows, fn row ->
        header
        |> Enum.zip(row)
        |> Map.new(fn
          {k, :expect_chars} -> {k, nil}
          kv -> kv
        end)
      end)

    {sheet, Kino.DataTable.new(maps)}
  end

Copy link
Contributor

Choose a reason for hiding this comment

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

also added this :expect_chars handling, because in many cases when the value is empty, that's what I get instead from the library, not sure if it's a behaviour we should change in xlsx_reader cc @xavier

Copy link
Contributor

Choose a reason for hiding this comment

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

@rodrigues what if we add two options to xlsx_reader? One to skip empty sheets and another to nillify expect_chars?

Copy link
Contributor

@rodrigues rodrigues Apr 25, 2024

Choose a reason for hiding this comment

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

that sounds good to me, sheet_names/2 could handle skip_empty: true.

about :expect_chars, it sounds to me like maybe it should be nil by default, but I don't know the reasoning for returning it today.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rodrigues would you open up issues/send PRs to xlsx_reader, please? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

yep I'll find some time to work on these two 👌

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.

5 participants