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

Remove messytables dependency from xlsx import #289

Merged
merged 1 commit into from
Sep 28, 2022

Conversation

yohanboniface
Copy link
Contributor

Next step after #265.

⚠️ Work in progress ⚠️

Some points to notice, and to be discussed/arbitrated:

  • I've removed complex() call from _is_numeric helper, as complex("j") returns True, and I'm not exactly sure how to work around this and how much it's important to recognize a complex number here

With the complex check, this table headers will not be recognized (because j is a valid complex value):

a b c d e f g h i j
9:14 PM 9/19/2019 1:10 10:20:30 100:20:30 vendredi 19 janvier 1900 7/4/1776 7/4/1976 samedi 30 décembre 1899 #FMT
  • I've ported from messytable the idea of trying to select only rows enough cells as header candidate (instead of the blindly first row with at least one non empty cell; this is the behaviour we had until now anyway, because we were using headers_guess from messytable, and not our own version.

With our version this table:

Title
A B C
1 4 7
2 5 8
3 6 9

Will be imported with Title,, as headers, instead of A,B,C.

  • test_excel_single_merged_cell output table is not respected, and I'm not sure to understand exactly if the first structure was explicitly expected or not, so I'll need some lights here

Here is the fixture table:

image

Before:

image

After:

image

Thanks in advance for your feedback!

cc @alexmojaki

@alexmojaki
Copy link
Contributor

alexmojaki commented Sep 23, 2022

Thanks @yohanboniface, this looks great, and I'm happy with your decisions. Is it still a work in progress?

@yohanboniface
Copy link
Contributor Author

Ah, sorry, well, it's a work in progress in the sense that it needs a review and certainly some arbitration :)

@alexmojaki
Copy link
Contributor

So can I merge it?

@yohanboniface
Copy link
Contributor Author

yohanboniface commented Sep 23, 2022

Yes, if you agree with the points I mentioned in the description! :)

@alexmojaki alexmojaki merged commit 0875153 into gristlabs:main Sep 28, 2022
@alexmojaki
Copy link
Contributor

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants