Skip to content

ENH: Support for specifying col names for col_space #28929

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

Closed

Conversation

VijayantSoni
Copy link
Contributor

@VijayantSoni VijayantSoni commented Oct 11, 2019

TO UPDATE FOLLOWING THINGS

@pep8speaks
Copy link

Hello @VijayantSoni! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 127:89: E501 line too long (92 > 88 characters)

@jbrockmendel
Copy link
Member

Can you add test(s) for the changed behavior

@simonjayhawkins simonjayhawkins added Enhancement IO HTML read_html, to_html, Styler.apply, Styler.applymap labels Oct 11, 2019
Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

@VijayantSoni Thanks for the PR.

@@ -533,6 +533,7 @@ def __init__(
frame: "DataFrame",
columns: Optional[Sequence[str]] = None,
col_space: Optional[Union[str, int]] = None,
col_space_cols: Optional[Sequence[str]] = None,
Copy link
Member

Choose a reason for hiding this comment

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

rather than adding an additional argument, can you make the behavior similar to the formatters argument. (and reuse code would be great). i.e. also accept a list or a dict. (or Sequence or Mapping)

if a list, the number of elements in the list should be the same as the number of columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @simonjayhawkins , Sorry I didn't get your comment. The following statement seems confusing to me.

rather than adding an additional argument, can you make the behavior similar to the formatters argument

Did you mean to NOT add the additional argument col_space_cols and use existing formatters argument for this change or add an argument similar to formatters, which accepts a list or a dict.

Also,

if a list, the number of elements in the list should be the same as the number of columns

Not sure if I understand this, if we want to change width of only a single column, we can pass only that column's name in the list, right ?

Please let me know if I missing something here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @simonjayhawkins , did you get a chance to look at the my concern ? I think I need to make the new argument similar to the formatters argument, but would still wait for your confirmation/opinion. Thanks!

@WillAyd
Copy link
Member

WillAyd commented Nov 7, 2019

@VijayantSoni are you still working on this? Agreed with @simonjayhawkins this shouldn't be a new keyword. You can model col_space off of what formatters currently accepts

@VijayantSoni
Copy link
Contributor Author

@WillAyd yeah, will work on changes.

@VijayantSoni
Copy link
Contributor Author

@WillAyd @simonjayhawkins Hey guys, just to update, I won't be able to work on this for another 25-20 days. Please feel free to take it up. If this is still open after that time, I will continue.

@simonjayhawkins
Copy link
Member

@WillAyd @simonjayhawkins Hey guys, just to update, I won't be able to work on this for another 25-20 days. Please feel free to take it up. If this is still open after that time, I will continue.

@VijayantSoni Thanks for looking into this. I'll close for now to clear the queue. can always re-open. ping when you wish to continue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement IO HTML read_html, to_html, Styler.apply, Styler.applymap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataFrame.to_html col_space parameter to change width of a specific column only
5 participants