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

Fixing exception for Table formatting type mismatch #6385

Merged
merged 8 commits into from Aug 18, 2017

Conversation

@bsipocz
Copy link
Member

bsipocz commented Jul 21, 2017

Rebased version of #3261

Based on the comment #3261 (comment) I think that PR was good to go, just got merge conflicts.

fixes #2868

@astropy-bot

This comment has been minimized.

Copy link

astropy-bot bot commented Jul 21, 2017

Hi there @bsipocz 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

@bsipocz bsipocz requested a review from taldcroft Jul 21, 2017
@bsipocz bsipocz force-pushed the bsipocz:table_rebase_3261 branch from 35c3dd0 to 9793532 Jul 21, 2017
@bsipocz

This comment has been minimized.

Copy link
Member Author

bsipocz commented Jul 22, 2017

this is a weird one, apparently locally it's seemingly randomly fails with E AttributeError: 'Column' object has no attribute '_name' or passes without issues.

@taldcroft

This comment has been minimized.

Copy link
Member

taldcroft commented Jul 22, 2017

@bsipocz - thanks for picking this up. Please see comments:

#3261 (comment)
#3261 (comment)
#3261 (comment)

Copy link
Member

taldcroft left a comment

OK, apart from my comments (and the bot) looks generally good.

@@ -701,7 +740,7 @@ def _copy_attrs(self, obj):
"""
Copy key column attributes from ``obj`` to self
"""
for attr in ('name', 'unit', 'format', 'description'):
for attr in ('_name', '_unit', '_format', 'description'):

This comment has been minimized.

Copy link
@taldcroft

taldcroft Jul 22, 2017

Member

This seems unrelated to this PR, but more importantly seems related to test failures. I suggest reverting.

return

try:
prev_format = self._format # save current format string

This comment has been minimized.

Copy link
@taldcroft

taldcroft Jul 22, 2017

Member

prev_format = getattr(self, '_format', None)

@@ -5,6 +5,7 @@
from ..extern.six import text_type
from ..extern.six.moves import zip, range

import itertools

This comment has been minimized.

Copy link
@taldcroft

taldcroft Jul 22, 2017

Member

Unused import?

# test whether it formats without error exemplarily
self.pformat(max_lines=1)

if self.dtype.kind == 'O':

This comment has been minimized.

Copy link
@taldcroft

taldcroft Jul 22, 2017

Member

Even though this is formally correct to do the full pre-checking here, I think it is better to not have a very expensive operation. One could end up formatting a billion lines just to set theformat attribute, which is not a good thing.

cherti and others added 5 commits Oct 25, 2014
Column implementation in master.  This also made a few changes to
methods like __array_finalize__ and __setstate__ to not go through the
setters for name, unit, and format--these are copying/recreating
Columns from existing objects that should presumably already have valid
values for those attributes.

This also addresses my comment on the original PR here:
#3055 (comment)

Finally, in order for this to pass testing several existing tests needed
to be updated, now that we no longer allow a Column with an invalid
format to be created in the first place (a change I support).
@bsipocz bsipocz force-pushed the bsipocz:table_rebase_3261 branch from 9793532 to dfd7e7a Aug 11, 2017
@bsipocz

This comment has been minimized.

Copy link
Member Author

bsipocz commented Aug 11, 2017

@taldcroft - I've mostly addressed your comments after redid the PR rebase fixes. I think most of the original comments were already dealt with, but please give it another review when you have time.

I'll add a changelog, too once the CI passed.

Copy link
Member

taldcroft left a comment

Just the last long-standing comment about the change from ValueError to TypeError. This should be reverted.

Thanks for sticking with this!

@@ -133,8 +133,8 @@ def _auto_format_func(format_, val):
break
else:
# None of the possible string functions passed muster.
raise ValueError('Unable to parse format string {0}'
.format(format_))
raise TypeError('unable to parse format string {0} for its '

This comment has been minimized.

Copy link
@taldcroft

taldcroft Aug 12, 2017

Member

Keep as ValueError. This is consistent with Python behavior. The problem is not the type of the column but the value of the format string.

try:
yield format_col_str(idx)
except TypeError:
raise TypeError(

This comment has been minimized.

Copy link
@taldcroft

taldcroft Aug 12, 2017

Member

Catch ValueError and raise ValueError.

bsipocz added 2 commits Aug 13, 2017
@bsipocz

This comment has been minimized.

Copy link
Member Author

bsipocz commented Aug 13, 2017

@taldcroft - Done.

Thanks for sticking with this!

No problem, this was a low hanging fruit for my occasional triaging of reducing the number of open issues.

@taldcroft

This comment has been minimized.

Copy link
Member

taldcroft commented Aug 14, 2017

@bsipocz - Sorry, I had not noticed before this was milestoned for 2.0.2, but I think it should be 3.0. The changelog could be something like:

New feature:
Improved exception handling and error messages when column ``format`` 
attribute is incorrect for the column type.

API change: 
When setting the column ``format`` attribute the value is now immediately validated. 
Previously one could set to any value and it was only checked when actually formatting the
column.
Copy link
Member

taldcroft left a comment

@bsipocz - if you double-check the changelog (which I just edited) then this should be good to merge once tests pass.

@bsipocz

This comment has been minimized.

Copy link
Member Author

bsipocz commented Aug 18, 2017

Thanks @taldcroft! I meant to come back to this, but it slipped through...

@bsipocz bsipocz merged commit 53c28b3 into astropy:master Aug 18, 2017
5 checks passed
5 checks passed
astropy-bot All checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 83.624%
Details
@taldcroft

This comment has been minimized.

Copy link
Member

taldcroft commented Aug 18, 2017

I meant to come back to this, but it slipped through...

I'm very familiar with this phenomenon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.