Fixing exception for Table formatting type mismatch by bsipocz · Pull Request #6385 · astropy/astropy

bsipocz · 2017-07-21T22:31:44Z

Rebased version of #3261

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

astropy-bot · 2017-07-21T22:31:59Z

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 · 2017-07-22T00:05:42Z

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 · 2017-07-22T10:55:33Z

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

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

taldcroft

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

taldcroft · 2017-07-22T10:52:44Z

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


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

taldcroft · 2017-07-22T10:58:52Z

+            self._format = None
+            return
+
+        try:


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

taldcroft · 2017-07-22T11:01:24Z

 from ..extern.six import text_type
 from ..extern.six.moves import zip, range

+import itertools


Unused import?

taldcroft · 2017-07-22T14:17:26Z

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


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.

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: astropy#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).

…dded a docstring for Column.format

bsipocz · 2017-08-11T22:13:21Z

@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.

taldcroft

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

Thanks for sticking with this!

taldcroft · 2017-08-12T11:38:36Z

                # 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 '


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.

taldcroft · 2017-08-12T11:39:35Z

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


Catch ValueError and raise ValueError.

bsipocz · 2017-08-13T17:55:13Z

@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 · 2017-08-14T01:03:34Z

@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.

taldcroft

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

bsipocz · 2017-08-18T17:26:51Z

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

taldcroft · 2017-08-18T18:45:46Z

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

I'm very familiar with this phenomenon.

bsipocz added Affects-release Bug table labels Jul 21, 2017

bsipocz requested a review from taldcroft July 21, 2017 22:32

bsipocz force-pushed the table_rebase_3261 branch from 35c3dd0 to 9793532 Compare July 21, 2017 23:53

taldcroft requested changes Jul 22, 2017

View reviewed changes

cherti and others added 5 commits August 9, 2017 16:15

changed thrown error to TypeError to indicate type mismatch

0b31dc8

Addressed feedback from @taldcroft here: astropy#3261 (comment) and a…

587aa76

…dded a docstring for Column.format

Fixing PR after rebase to pass tests

1fab752

Addressing review comments

dfd7e7a

bsipocz force-pushed the table_rebase_3261 branch from 9793532 to dfd7e7a Compare August 11, 2017 21:23

bsipocz added this to the v2.0.2 milestone Aug 11, 2017

taldcroft requested changes Aug 12, 2017

View reviewed changes

bsipocz added 2 commits August 13, 2017 18:52

Raise ValueError rather than TypeError

4689cb1

Adding changelog

8a2140b

taldcroft modified the milestones: v3.0.0, v2.0.2 Aug 14, 2017

Update changelog entries (move to 3.0 instead of 2.0)

cd01c6e

taldcroft approved these changes Aug 16, 2017

View reviewed changes

bsipocz merged commit 53c28b3 into astropy:master Aug 18, 2017

taldcroft mentioned this pull request Nov 6, 2017

Fix problem where bad column format is not reverted #6812

Merged

bsipocz deleted the table_rebase_3261 branch September 16, 2024 21:21

Conversation

bsipocz commented Jul 21, 2017

Uh oh!

astropy-bot Bot commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Jul 22, 2017

Uh oh!

taldcroft commented Jul 22, 2017

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

taldcroft Jul 22, 2017

Choose a reason for hiding this comment

Uh oh!

taldcroft Jul 22, 2017

Choose a reason for hiding this comment

Uh oh!

taldcroft Jul 22, 2017

Choose a reason for hiding this comment

Uh oh!

taldcroft Jul 22, 2017

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Aug 11, 2017

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

taldcroft Aug 12, 2017

Choose a reason for hiding this comment

Uh oh!

taldcroft Aug 12, 2017

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Aug 13, 2017

Uh oh!

taldcroft commented Aug 14, 2017

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Aug 18, 2017

Uh oh!

taldcroft commented Aug 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

astropy-bot Bot commented Jul 21, 2017 •

edited

Loading