-
-
Notifications
You must be signed in to change notification settings - Fork 593
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: support column_width in xlsx format #516
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #516 +/- ##
==========================================
+ Coverage 90.84% 91.11% +0.27%
==========================================
Files 28 28
Lines 2664 2702 +38
==========================================
+ Hits 2420 2462 +42
+ Misses 244 240 -4 ☔ View full report in Codecov by Sentry. |
Hello @hugovk! Sorry for ping, but I just want to ask if you have any time for review and merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping! Please could you add some tests that use the different possible values?
Co-authored-by: Hugo van Kemenade <[email protected]>
@hugovk Thank you for the review! I've added the test on check the new feature, checkout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general comments. And please could we have three separate test cases that call export_set
with different values for column_width
: None
, an integer and "adaptive"
.
That way we see each branch of your code is running without error, and it will be reflected in the coverage.
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Thank you for such usefull comment! I've found a bug with new tests. Your comments are pushed, alongside with all three tests. What do you think on it? |
Co-authored-by: Hugo van Kemenade <[email protected]>
Thank you for your comment! I believe this significantly improves redability. Could please take a look if the PR is fine now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! A couple of minor suggestions.
|
||
|
||
|
||
.. versionchanged:: 3.3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.3.0 has been released, let's target 3.4.0:
.. versionchanged:: 3.3.0 | |
.. versionchanged:: 3.4.0 |
raise ValueError(f"Unsupported value `{column_width}` passed to `column_width` " | ||
"parameter. It supports 'adaptive' or integer values") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like this, to be closer to existing exceptions?
And let's start assigning exception messages to a variable first, for the reasons set out at https://github.com/henryiii/flake8-errmsg
raise ValueError(f"Unsupported value `{column_width}` passed to `column_width` " | |
"parameter. It supports 'adaptive' or integer values") | |
msg = ( | |
f"Invalid value for column_width: {column_width}. " | |
f"Must be 'adaptive' or an integer." | |
) | |
raise ValueError(msg) |
Hi there, I would find this enhancement useful. Do you mind to take this last step to add this feature? |
Hi! I won't be able to finish this PR, so feel free to do that! |
This PR allows user to specify column width in XLXS format. For it has a default value, which is not suitable for long values in dataset. As a result, it is not convenient to look these tables. Also, it eliminates questions like #318 in future
I didn't find tests for exporting, so omitted this. Let me know if they are required