Skip to content

column headers now row -1 of the table #14544

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

Closed
wants to merge 1 commit into from

Conversation

swfiua
Copy link
Contributor

@swfiua swfiua commented Jun 13, 2019

PR Summary

This addresses #12931

It is an API change since cell indices for the body of the table are now the same regardless of whether the table has column headers.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@timhoffm
Copy link
Member

timhoffm commented Jun 13, 2019

Thanks a lot for your contributions. I really appreciate your work!

This is a breaking API change, we must consider how to handle this best. See #14545 (review) and following.

Ideas how to handle this the least painful way for users are very welcome (even though an index change will likely be a hard break one way or the other).

BTW: Are you planning on adding more changes to Table? If so, we might consider a complete rewrite, which might be easier to transition from an API compatibility point of view.

@swfiua
Copy link
Contributor Author

swfiua commented Jun 14, 2019

Thanks, trust me it is not fun digging into the 15 year old code I wrote ;)

All these recent table related pull requests involve some degree of table breakage.

Re: am I planning on adding more changes to the table. I am leaning towards the suggestion of creating a separate mpltable project, which would give a lot more freedom to break things.

It also gives a place to document the API differences which should help people move to the new table.

Once I have a working package then we could consider fixing matplotlib to do something like:

try:
from mpltable import table
# display warning that we are using mpltable?
except:
from . import table

Then anyone who wants to try the new table just has to install the mpltable package, if it breaks the fix is easy to: uninstall mpltable.

If we decide to go this route I think I would just close the existing pull requests and point people to the new package.

@timhoffm
Copy link
Member

Going with a clean separate apprach sounds reasonable to me. I think I would not import it to the exact name though. Can make for a lot of surprises on the user side; and is more difficult to cleanly deprecate, migrate and phase out. Going to think a bit more about a good way for that.

Overall this is a larger-scale API decision and @tacaswell should be involved. He also has some ideas how external packages could be integrated.

@swfiua
Copy link
Contributor Author

swfiua commented Aug 5, 2019

This is fixed in my blume project, which provides a fixed up table widget with a number of other enhancements:

https://github.com/swfiua/blume/

On PyPi as blume.

@timhoffm
Copy link
Member

timhoffm commented Aug 5, 2019

Closing as too much of an API change. Since there are several issues around tables, a new separate table class is the way forward. Currently, this is realized in https://github.com/swfiua/blume.

@swfiua please report back in case you think this PR is still of value.

@timhoffm timhoffm closed this Aug 5, 2019
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.

2 participants