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

bug fix related #5479 #6047

Merged
merged 12 commits into from Mar 7, 2016
Merged

bug fix related #5479 #6047

merged 12 commits into from Mar 7, 2016

Conversation

ryanbelt
Copy link
Contributor

Table: auto_set_column_width not working #5479

As before, it append the whole parameter(col) into the _autoColumns even col is a list object.
at table.py line 490 inside function _update_position

for col in self._autoColumns:
      self._auto_set_column_width(col, renderer)

_auto_set_column_width are not able to manipulate with the index number as list rather then int passed from line490.

So, I decide to append number to _autoColumns as usual if and only if the parameter is integer. If it is a list, we will append each integer inside the parameter into _autoColumns

@@ -410,7 +410,13 @@ def _do_cell_alignment(self):

def auto_set_column_width(self, col):

self._autoColumns.append(col)
# col is a list of column index
if isinstance(col, list):
Copy link
Member

Choose a reason for hiding this comment

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

This is probably better to do as

try:
    self._autoColumns.extend(cell)
except CORRECT_EXCEPTION:
    self._autoColumns.append(col)

@ryanbelt
Copy link
Contributor Author

I have change the format from type checking to exception checking.
This would allow not only list obj as the parameter, but also any kinds of iterable object such tuple etc.

table.auto_set_column_width(-1) # Default input
table.auto_set_column_width([-1,0,1]) # List input
table.auto_set_column_width((-1,0,1)) # Tuple input

except (TypeError, AttributeError):
self._autoColumns.append(col)
else:
for cell in col:
Copy link
Member

Choose a reason for hiding this comment

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

This indentation is funny and you can use extend.

@tacaswell tacaswell mentioned this pull request Feb 25, 2016
@ryanbelt
Copy link
Contributor Author

I have just seen the comment from #6059. I am going to add the specific test case for this PR.

@ryanbelt
Copy link
Contributor Author

@tacaswell ,
Test case for this specific bug has been added. Bascially testing every single column be able to auto_width. By using List, Tuple and single integer as input. I think those are the main issue on this bug.
Thank you

@dashed
Copy link
Contributor

dashed commented Feb 27, 2016

Strings are also iterable. Should that be also supported?

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Feb 28, 2016
@ryanbelt
Copy link
Contributor Author

@dashed , inside the _auto_set_column_width. if the object inside the _autoColumns is not an integer(index), It will not able to do the width adjustment and remain as default width (will causing content go out the box). So, it will not crash the program. It just skip all those string object until next integer found or stop at the end of the _autoColumns.

@dashed
Copy link
Contributor

dashed commented Feb 28, 2016

@ryanbelt Ah ok. Then should there be a test case for the behaviour you've just described?

@ryanbelt
Copy link
Contributor Author

@dashed , thank you for the tips. Unexpected test case for iterable input as string has been added for this bug.

@@ -410,7 +410,15 @@ def _do_cell_alignment(self):

def auto_set_column_width(self, col):

self._autoColumns.append(col)
# check for col possibility on iteration
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be worded in another way. Suggestion:

If col is iterable, concatenate to self._autoColumns. Otherwise, append it to self._autoColumns.

@dashed
Copy link
Contributor

dashed commented Feb 28, 2016

@ryanbelt Awesome. 👍 Unfortunately you beat us to the punch on this PR for d01. But I'll throw some suggestions your way for this PR 😄 .

tacaswell added a commit that referenced this pull request Mar 7, 2016
FIX: table auto_column_width

fix #5479
@tacaswell tacaswell merged commit 963e51d into matplotlib:master Mar 7, 2016
@tacaswell
Copy link
Member

Thanks!

Would one of you @dashed or @ryanbelt be willing to write a docstring for auto_set_column_width?

@dashed
Copy link
Contributor

dashed commented Mar 7, 2016

I'll leave that for @ryanbelt 😄

@ryanbelt
Copy link
Contributor Author

ryanbelt commented Mar 7, 2016

        """Given column indexs in either List, Tuple or int. Will be able to
        autonatically set the columns into optimal sizes.
        """

@tacaswell ,
How should I added into the table.py since this pull request is closed?
Thank you.

@tacaswell
Copy link
Member

@ryanbelt Make a new pull request. They are (more-or-less) free and the smaller the change, the easier it is to review.

Could you also explain the meaning of the values that are expected to be passed in?

@ryanbelt ryanbelt mentioned this pull request Mar 7, 2016
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Aug 3, 2023
When iterables were added in matplotlib#6047, the test added a string. Typing
correctly points out that that is not accepted, and in fact it does not
do anything (as shown in the test image) because column keys are ints,
not strings.
@QuLogic QuLogic mentioned this pull request Aug 3, 2023
3 tasks
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Aug 3, 2023
When iterables were added in matplotlib#6047, the test added a string. Typing
correctly points out that that is not accepted, and in fact it does not
do anything (as shown in the test image) because column keys are ints,
not strings.
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.

None yet

4 participants