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

Problem parsing magnitude that starts with "0" #614

Open
cpascual opened this issue Jan 24, 2018 · 14 comments
Open

Problem parsing magnitude that starts with "0" #614

cpascual opened this issue Jan 24, 2018 · 14 comments

Comments

@cpascual
Copy link
Contributor

Hi, I am not sure if this is a bug, but at least it is quite unexpected to me:

print Quantity('123').magnitude  # --> 123
print Quantity('0123').magnitude  # --> 0  
cpascual pushed a commit to cpascual/taurus that referenced this issue Jan 24, 2018
The TaurusValueLineEdit is a ffected by a bug in pint
(hgrecco/pint#614). Work around it by
stripping any leading zeroes from a value string to be passed to
pint's Quantity constructor.
@cpascual
Copy link
Contributor Author

If I get confirmation that this is unwanted behaviour, I can try to submit a PR for changing it

@dalito
Copy link
Contributor

dalito commented Jan 24, 2018

I find the result unexpected, too. But what is the wanted behaviour? Should pint match the Python interpreter? That may be surprising for newcomers in case of Python 2.x:

Python 2.7.14 (v2.7.14:84471935ed, Sep 16 2017, 20:19:30) [MSC v.1500 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.                        
>>> 0123                                                                                      
83     
Python 3.6.4 (v3.6.4:d48eceb, Dec 19 2017, 06:54:40) [MSC v.1900 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> 0123
  File "<stdin>", line 1
    0123
       ^
SyntaxError: invalid token
>>>

@cpascual
Copy link
Contributor Author

Yes, I would expect pint to strip units and then treat the rest just as python's eval would.

So, in python 2:

Quantity(0123)    # returns <Quantity(83, 'dimensionless')> ... OK
Quantity(0o123)  # returns <Quantity(83, 'dimensionless')> ... OK
Quantity(0123.)   # returns  <Quantity(123.0, 'dimensionless')> ... OK
Quantity('0123')  # returns  <Quantity(0, 'dimensionless')>
                  # ... but should return:  <Quantity(83, 'dimensionless')>
Quantity('0o123')  # raises UndefinedUnitError: 'o123m' is not defined in the unit registry.
                  # ... but should return:  <Quantity(83, 'dimensionless')>
Quantity('0123m') # returns  <Quantity(0, 'meter')>
                  # ... but should return: <Quantity(83, 'meter')>
Quantity('0123.')   # returns  <Quantity(123.0, 'dimensionless')>  OK

And in Python 3:

Quantity(0123)  # raises SyntaxError: invalid token ... OK
Quantity(0o123)  # returns <Quantity(83, 'dimensionless')>  ... OK
Quantity(0123.)   # returns  <Quantity(123.0, 'dimensionless')> ... OK
Quantity('0123')  # returns  <Quantity(0, 'dimensionless')>
                   # ... but should raise SyntaxError: invalid token   
Quantity('0o123')  # raises UndefinedUnitError: 'o123m' is not defined in the unit registry.
                   # ... but should return:  <Quantity(83, 'dimensionless')>
Quantity('0123m') # returns  <Quantity(0, 'meter')>  
                    # ... but should return: <Quantity(83, 'meter')>
Quantity('0123.')   # returns  <Quantity(123.0, 'dimensionless')>  ... OK

@cpascual
Copy link
Contributor Author

cpascual commented Jan 24, 2018

The above is IMHO the clearest behaviour (in the sense that one can refer to python's eval for the expected output)

But, personally, I would not object if Quantity('0123') returned <Quantity(123, 'dimensionless')> in both python 2 and 3

@dalito
Copy link
Contributor

dalito commented Jan 24, 2018

What you propose, looks all good to me.

@hgrecco
Copy link
Owner

hgrecco commented Mar 23, 2018

Good to find you both of you in one issue! I would like to invite you to have commit rights to this. I have been very busy lately (I am sure you notice, sorry) To keep pint in good shape we need new blood and new ideas. You have contributed with code, ideas and discussion to pint and it will be great if your would like to join

@cpascual
Copy link
Contributor Author

@hgrecco: I am glad to join, but I want to be up-front (at the risk of sounding selfish): I cannot commit much time to maintaining another project so if I join it would be mostly to focus on fixing/implementing those features that affect my main project (http://github.com/taurus-org). If you are ok with that, I am in!

@dalito
Copy link
Contributor

dalito commented Mar 23, 2018

@hgrecco: I am also willing to join but will be very limited in the time that I can commit to pint. In the end three people with little time will still be better than just one.

@hgrecco
Copy link
Owner

hgrecco commented Mar 23, 2018

Awesome. My idea is to find a group of around five people that have contributed regularly to pint and to provide access. I perfectly understand (and suffer my self) the time constrains and I agree that a collaborative effort. Pint is a project that service others and therefore it makes perfect sense that bugs will be discovered when integrated in other project and features will be designed with certain projects in mind. So .. let's go ahead.

@dopplershift
Copy link
Contributor

@hgrecco I'm happy to help where I can as well. The core library I'm responsible heavily leverages pint, so it's very important that it keep progressing.

@hgrecco
Copy link
Owner

hgrecco commented Mar 23, 2018

I am using bors to merge PR in Pint. It is very nice as it provides a consistent and safe way to merge PR. You can read the docs here

TL;DR Just comment any PR that you want to merge with bors r+ to tell bors that you have approved it. You can do it with multiple PRs and bors will merge each of them sequentially testing them against the new master after each merge.

@hgrecco
Copy link
Owner

hgrecco commented Mar 26, 2018

Would you like to find a PR that you like / feel comfortable of merging and merging through bors so we can see that everything is fine?

@cpascual
Copy link
Contributor Author

cpascual commented Apr 5, 2018

Hi @hgrecco . I had a look at the open PRs, but I didn't find any with which I am confident enough to push. However, I am personally interested in seeing #577 fixed, so I am going to try to do a PR for it. And then if someone else reviews it and gives the "ok" I will test the bors workflow with that one. Is that ok?

@hgrecco
Copy link
Owner

hgrecco commented Apr 5, 2018

sounds perfect. Go for it!

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

No branches or pull requests

4 participants