Skip to content

Updated some ValueError exceptions to throw InconsistentTableError. - #7425

Merged
bsipocz merged 3 commits into
astropy:masterfrom
humnaawan:io-ascii-exceptions
May 15, 2018
Merged

Updated some ValueError exceptions to throw InconsistentTableError.#7425
bsipocz merged 3 commits into
astropy:masterfrom
humnaawan:io-ascii-exceptions

Conversation

@humnaawan

@humnaawan humnaawan commented May 3, 2018

Copy link
Copy Markdown
Contributor

Fixes #7413

@astropy-bot

astropy-bot Bot commented May 3, 2018 •
edited
Loading

Copy link
Copy Markdown

Hi there @humnaawan 👋 - 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.

@bsipocz
bsipocz requested a review from taldcroft May 3, 2018 14:42
@bsipocz

bsipocz commented May 3, 2018

Copy link
Copy Markdown
Member

@taldcroft - We don't need a changelog for this, right?

@bsipocz bsipocz added this to the v3.1 milestone May 3, 2018
@taldcroft

taldcroft commented May 3, 2018

Copy link
Copy Markdown
Member

@bsipocz - this seems like it deserves mention as an API change, although it should be back-compatible since InconsistentTableError derives from ValueError.

I haven't actually looked at this, but want to make sure it doesn't get in the way of #5578, which I think can be ready to merge pretty soon.

taldcroft
taldcroft previously requested changes May 3, 2018
View reviewed changes

@taldcroft taldcroft left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks generally OK, just the two comments I made that should be addressed.

Comment thread astropy/io/ascii/core.py Outdated
from ...utils.data import get_readable_fileobj
from . import connect

from . import core

@taldcroft taldcroft May 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't import yourself.

@humnaawan humnaawan May 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, can you say how I should be calling InconsistentTableError?

@humnaawan humnaawan May 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've removed the import; sorry, I didnt realize I was importing core in core.py.

@humnaawan
humnaawan force-pushed the io-ascii-exceptions branch from f942cca to d4e68ad Compare May 3, 2018 19:07
@bsipocz

bsipocz commented May 3, 2018

Copy link
Copy Markdown
Member

@taldcroft - This is ready to go and would be nice to merge (as it was done during hackday PR-ing), but it's understandable if you want to held it up due to #5578

@taldcroft

taldcroft commented May 4, 2018

Copy link
Copy Markdown
Member

@bsipocz - can you check if this PR will introduce a conflict? I'm just especially wary since #5578 is not actually rebase-able (at least by me) at this point.

taldcroft
taldcroft reviewed May 4, 2018
View reviewed changes
Comment thread CHANGES.rst Outdated
astropy.io.ascii
^^^^^^^^^^^^^^^^

- Updated some ValueError exceptions to throw InconsistentTableError. [#7425]

@taldcroft taldcroft May 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can give a bit more detail and guidance to users like so:

Changed a number of ``ValueError`` exceptions to ``InconsistentTableError`` in places
where the exception is related to parsing a table which is inconsistent with the
specified table format.  Note that ``InconsistentTableError`` inherits from ``ValueError``
so no user code changes are required. [#7425]

@bsipocz

bsipocz commented May 4, 2018

Copy link
Copy Markdown
Member

@taldcroft - If I try to cherry-pick these commits on top of the current #5578 then there is one tiny and trivial conflict that originates from a whitespace issue, but otherwise it's a clean on.
I can give the rebase a try, it seemed very non-trivial at first, but maybe with some squashing first it can be simplified a bit.

@taldcroft

taldcroft commented May 4, 2018

Copy link
Copy Markdown
Member

@bsipocz - I did try the "squashing first" technique (which I generally find helpful), but strangely got the same conflicts. I.e. I tried git rebase -i <commit_before_first_5578_commit>, and still got conflicts. This made no sense. Then I even tried squashing with git rebase -i <first_5578_commit>, and that gave (after a long wait) every commit from the master merge and I just aborted.

But your git fu may yet be powerful enough!!

dhomeier added a commit to dhomeier/astropy that referenced this pull request May 9, 2018
@dhomeier
Update to InconsistentTableError in test_read_with_names_arg()
ace9f4e 
Changed ValueError exception in check_column_names() and associated
test to InconsistentTableError; changes in core.py may overlap with
PR astropy#7425.
bsipocz pushed a commit to bsipocz/astropy that referenced this pull request May 10, 2018
@dhomeier @bsipocz
Update to InconsistentTableError in test_read_with_names_arg()
1276f20 
Changed ValueError exception in check_column_names() and associated
test to InconsistentTableError; changes in core.py may overlap with
PR astropy#7425.
@bsipocz

bsipocz commented May 13, 2018

Copy link
Copy Markdown
Member

Now that #5578 is merged, this PR indeed ended up with conflicts. I'm going ahead and rebase and address the comment above.

@bsipocz
bsipocz force-pushed the io-ascii-exceptions branch from d4e68ad to 7e4a991 Compare May 13, 2018 13:20
@bsipocz bsipocz added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label May 13, 2018
bsipocz
bsipocz approved these changes May 15, 2018
View reviewed changes

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review comments has been addressed and the other PR holding this up got merged.

@bsipocz
bsipocz dismissed taldcroft’s stale review May 15, 2018 22:52

Comments has been addressed.

@bsipocz
bsipocz merged commit 3986964 into astropy:master May 15, 2018
@bsipocz

bsipocz commented May 15, 2018

Copy link
Copy Markdown
Member

Thank you @humnaawan!

astromancer pushed a commit to astromancer/astropy that referenced this pull request Sep 18, 2018
@dhomeier astromancer
Update to InconsistentTableError in test_read_with_names_arg()
39a93e5 
Changed ValueError exception in check_column_names() and associated
test to InconsistentTableError; changes in core.py may overlap with
PR astropy#7425.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taldcroft taldcroft taldcroft left review comments
@bsipocz bsipocz bsipocz approved these changes

Assignees

No one assigned

Labels

io.ascii zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now.

Projects

None yet

Milestone

v3.1

Development

Successfully merging this pull request may close these issues.

3 participants

@humnaawan @bsipocz @taldcroft