Skip to content

Fix problem where bad column format is not reverted#6812

Merged
mhvk merged 1 commit intoastropy:masterfrom
taldcroft:table-format-error
Nov 7, 2017
Merged

Fix problem where bad column format is not reverted#6812
mhvk merged 1 commit intoastropy:masterfrom
taldcroft:table-format-error

Conversation

@taldcroft
Copy link
Copy Markdown
Member

@taldcroft taldcroft commented Nov 6, 2017

#6385 added code to validate format when it is set, but in the mean time the expected TypeError got changed to a ValueError. The unit testing of this just checks for a ValueError but does not check that the format got reverted.

        try:
            # test whether it formats without error exemplarily
            self.pformat(max_lines=1)
        except TypeError as err:
            # revert to restore previous format if there was one
            self._format = prev_format
            raise ValueError(
                "Invalid format for column '{0}': could not display "
                "values in this column using this format ({1})".format(
                    self.name, err.args[0]))

@taldcroft taldcroft added this to the v2.0.3 milestone Nov 6, 2017
@taldcroft taldcroft requested a review from mhvk November 6, 2017 21:29
@astropy-bot
Copy link
Copy Markdown

astropy-bot bot commented Nov 6, 2017 •
edited
Loading

Hi there @taldcroft 👋 - 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 labeled 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! 👍

If there are any issues with this message, please report them here.

@taldcroft taldcroft mentioned this pull request Nov 6, 2017
Closed
mhvk
mhvk reviewed Nov 6, 2017
View reviewed changes
astropy/table/column.py Outdated
# test whether it formats without error exemplarily
self.pformat(max_lines=1)
except TypeError as err:
except (TypeError, ValueError) as err:
Copy link
Copy Markdown
Contributor

@mhvk mhvk Nov 6, 2017

Choose a reason for hiding this comment

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

Shouldn't we just catch any exception here? I.e., except Exception as err?

Also, I may have worried about this before, but what happens if the first element is masked? I vaguely recall writing delayed formatting for that case, and max_lines=1 might then not catch it. Perhaps too much of a detail, though.

Copy link
Copy Markdown
Contributor

@mhvk mhvk Nov 6, 2017

Choose a reason for hiding this comment

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

So, I should read all my e-mail before commenting. So indeed I wrote about this 24 days ago (just tells you how long my memory lasts these days...)

Copy link
Copy Markdown
Contributor

@mhvk mhvk Nov 6, 2017

Choose a reason for hiding this comment

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

Bottom line: happy with this, though I'd use Exception - seems justified to be general in this case, since we re-raise it as a ValueError anyway.

@taldcroft
Copy link
Copy Markdown
Member Author

taldcroft commented Nov 7, 2017

@mhvk - OK made the change to Exception per your comment.

mhvk
mhvk approved these changes Nov 7, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

OK, all looks fine; merging...

@mhvk mhvk merged commit df04c13 into astropy:master Nov 7, 2017
@bsipocz bsipocz added Affects-dev PRs and issues that do not impact an existing Astropy release and removed Affects-release labels Nov 27, 2017
@bsipocz bsipocz modified the milestones: v2.0.3, v3.0.0 Nov 27, 2017
@bsipocz
Copy link
Copy Markdown
Member

bsipocz commented Nov 27, 2017

Since #6385 was milestoned to 3.0, this cannot be backported, and also doesn't yet affect release.

@bsipocz
Copy link
Copy Markdown
Member

bsipocz commented Dec 14, 2017

Since this is now affect-dev, do you still want the changelog entry? Currently it has been removed, but I can add it back is you feel the need for it.

@taldcroft taldcroft deleted the table-format-error branch February 25, 2019 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release Bug table

Projects

None yet

Milestone

v3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@taldcroft @bsipocz @mhvk