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

Adds fixed point numbers #36

Merged
merged 4 commits into from Jul 9, 2018

Conversation

Projects
None yet
4 participants
@rsercano
Copy link
Contributor

rsercano commented Jul 7, 2018

For #2 I've implemented two new types, would you consider checking it please ?
Also I've put some tests for both.

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jul 7, 2018

CLA assistant check
All committers have signed the CLA.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 7, 2018

Coverage Status

Coverage increased (+1.03%) to 88.435% when pulling 6f93d0d on rsercano:master into 2e5dad1 on aragon:master.

@luisivan luisivan requested a review from bingen Jul 9, 2018

let mXn = identifier.substr(5)

// Default to 256*80
if (!mXn || mXn.indexOf('x') === -1) mXn = '256x80'

This comment has been minimized.

@bingen

bingen Jul 9, 2018

Contributor

What's the point of the second part of the condition here? I don't think that things like fixedanythingelse should default to fixed256x80, and I guess this is what would happen with that condition, right?

This comment has been minimized.

@rsercano

rsercano Jul 9, 2018

Contributor

that's correct, is there a specific behavior you expect ? I'm now implementing it to return false just conditions like you mentioned

This comment has been minimized.

@bingen

bingen Jul 9, 2018

Contributor

Yes, I think false should be the correct return value for those irregular cases. Thanks!

let mXn = identifier.substr(6)

// Default to 256*80
if (!mXn || mXn.indexOf('x') === -1) mXn = '256x80'

This comment has been minimized.

@bingen

bingen Jul 9, 2018

Contributor

Same as before.

This comment has been minimized.

@rsercano

rsercano Jul 9, 2018

Contributor

implemented same as before :)

t.true(fixed.isType('fixed8x11'))
t.true(fixed.isType('fixed248x35'))
t.false(fixed.isType('fixed256x89'), 'Maximum length of N should be 80')
t.false(fixed.isType('fixed266x33'), 'Maximum length of M should be 80')

This comment has been minimized.

@bingen

bingen Jul 9, 2018

Contributor

This 80 should be a 256. Besides, a test for the minimum could be added.

This comment has been minimized.

@rsercano

rsercano Jul 9, 2018

Contributor

sorry for the typo, adding the requested test

t.true(fixed.isType('ufixed8x11'))
t.true(fixed.isType('ufixed248x35'))
t.false(fixed.isType('ufixed256x89'), 'Maximum length of N should be 80')
t.false(fixed.isType('ufixed266x33'), 'Maximum length of M should be 80')

This comment has been minimized.

@bingen

bingen Jul 9, 2018

Contributor

Same as before.

This comment has been minimized.

@rsercano

rsercano Jul 9, 2018

Contributor

++

t.true(fixed.isType('fixed248x35'))
t.false(fixed.isType('fixed256x89'), 'Maximum length of N should be 80')
t.false(fixed.isType('fixed266x33'), 'Maximum length of M should be 80')
t.false(fixed.isType('fixed129x33'), 'M should be divisible by 8')

This comment has been minimized.

@bingen

bingen Jul 9, 2018

Contributor

A test for something not starting with fixed could be added too. Also some tests for other weird combinations, like not having numbers after the prefix, e.g.: fixedAxB.

This comment has been minimized.

@rsercano

rsercano Jul 9, 2018

Contributor

added

t.true(fixed.isType('ufixed248x35'))
t.false(fixed.isType('ufixed256x89'), 'Maximum length of N should be 80')
t.false(fixed.isType('ufixed266x33'), 'Maximum length of M should be 80')
t.false(fixed.isType('ufixed129x33'), 'M should be divisible by 8')

This comment has been minimized.

@bingen

bingen Jul 9, 2018

Contributor

Same comments for more tests applies here too.

This comment has been minimized.

@rsercano

rsercano Jul 9, 2018

Contributor

added

@bingen bingen self-assigned this Jul 9, 2018

@bingen bingen added the enhancement label Jul 9, 2018

@rsercano

This comment has been minimized.

Copy link
Contributor

rsercano commented Jul 9, 2018

@bingen just added all requirements

isType (identifier) {
let mXn = identifier.substr(5)

if (!mXn || mXn.indexOf('x') === -1) return false

This comment has been minimized.

@bingen

bingen Jul 9, 2018

Contributor

Well, sorry to bother you again, but I was referring to the second condition only. An empty mXn should be valid, and actually, according to this it should default to 128x18. Could you do this little change?

This comment has been minimized.

@rsercano

rsercano Jul 9, 2018

Contributor

done :)

@bingen
Copy link
Contributor

bingen left a comment

Awesome. That was fast. Nice work with the tests. Could you address that minor comment I added? Then I think we are ready to roll. Thanks a lot for your contribution!

@rsercano

This comment has been minimized.

Copy link
Contributor

rsercano commented Jul 9, 2018

Just fixed as you requested, sorry for overlooking

@bingen

bingen approved these changes Jul 9, 2018

@bingen bingen merged commit bc29c22 into aragon:master Jul 9, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@bingen

This comment has been minimized.

Copy link
Contributor

bingen commented Jul 9, 2018

Awesome, thanks!!

@sohkai sohkai changed the title fixes #2 Adds fixed point numbers Nov 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment