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

Added Decimal to numeric types and allow petl.compat.numeric_types to be customized #568

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

blais
Copy link
Contributor

@blais blais commented Oct 6, 2021

This PR has two objectives:

  1. Make it possible to customize the list numeric_types by late access on the compat module
  2. Adding the decimal.Decimal type to the default list of numeric types.

The main purpose is for Decimal numbers to align-right in the vis/look() functions.

Checklist

  • I ran the unit tests with tox, for Python 3.9
  • I modified the only existing test involving numeric types
  • I checked for docs describing numeric types (there are none)

Anything else you need just let me know.

blais added 3 commits October 6, 2021 12:17
…types.

The current code does not right-align numeric types when converting to text.
numeric_types is defined in petl.compat.

The following customization (via monkey-patching, yes, I know) does not work, however:

import petl.compat
petl.compat.numeric_types = petl.compat.numeric_types + (Decimal,)
because in petl/util/vis.py it is accessed like this:

from petl.compat import numeric_types, text_type
and this module gets loaded as a side-effect of importing petl.

Changing it to this - everywhere it is used - would fix the issue:

from petl import compat
...
    isinstance(..., compat.numeric_types)
...
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 91.313% when pulling b75996d on blais:master into 8ee782e on petl-developers:master.

@juarezr juarezr added the Feature A nice to have thing that we don't have yet label Oct 6, 2021
@juarezr
Copy link
Member

juarezr commented Oct 7, 2021

@blais,

I think it would be nice to have this feature in petl.

I'm wondering if you already have some thoughts on what would be a proper API for unlocking this feature instead of monkey patching?

@blais
Copy link
Contributor Author

blais commented Oct 8, 2021

@blais,

I think it would be nice to have this feature in petl.

I'm wondering if you already have some thoughts on what would be a proper API for unlocking this feature instead of monkey patching?

Maybe providing a function to call instead, e.g., add_numeric_types() but then the whole thing could devolve into multiple functions for each of the customizable lists, mirroring a bunch of stuff for basically not every much protection. Personally I think just make those mutable is simpler, but perhaps with some instructions around it. It's a rarity, and in the patch I'm adding Decimal by default anyway. I'd leave it as is. Sometimes no API is the better API, and I think this is one of those cases.

@juarezr
Copy link
Member

juarezr commented Oct 8, 2021

Yes, I agree.

But I'm wondering if it's worth getting rid of compat like in #478 instead of bothering with this python2 compatibility hack.

Maybe merging the Decimal fix prior to a full source code conversion like #478.
What do you think about it?

@blais
Copy link
Contributor Author

blais commented Oct 8, 2021

Yes, I agree.

But I'm wondering if it's worth getting rid of compat like in #478 instead of bothering with this python2 compatibility hack.

Maybe merging the Decimal fix prior to a full source code conversion like #478. What do you think about it?

I think that's beyond my level of involvement with the library so far, it's a decision for the maintainers. My personal view, without the full context around version support expectations, is that yes, Python2 at this stage could be dropped. I mean it's been a very long while now. In any case, a list of numeric types would be required. So in my opinion it's an orthogonal issue.

@juarezr
Copy link
Member

juarezr commented Oct 8, 2021

Yes, I agree.
But I'm wondering if it's worth getting rid of compat like in #478 instead of bothering with this python2 compatibility hack.
Maybe merging the Decimal fix prior to a full source code conversion like #478. What do you think about it?

I think that's beyond my level of involvement with the library so far, it's a decision for the maintainers. My personal view, without the full context around version support expectations, is that yes, Python2 at this stage could be dropped. I mean it's been a very long while now. In any case, a list of numeric types would be required. So in my opinion it's an orthogonal issue.

As stated in the @alimanfoo petl's author roadmap and in the comments of #563, I think that the move is uncontroversial at this point.

The remaining issue is the lack of manpower for evolving the changes in #563, #336, a few other issues, and fix the packaging to pip, Conda, and readthedocs.org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature A nice to have thing that we don't have yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Late-import petl.compat.numeric_types in order to allow the user to customize it
3 participants