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

Fix parsing of fixed point numbers #28

Merged
merged 6 commits into from
Nov 23, 2020

Conversation

sfinkens
Copy link
Member

@sfinkens sfinkens commented Nov 12, 2020

This PR fixes the following problems with the parsing of fixed point numbers:

As suggested by @djhoese I added a new regular expression that is used if the format specifier includes width or precision. Finally, I updated the converter method to return floats.


Examples:

  1. "Naive" case without width and precision, matches any float:
>>> trollsift.parse('{foo:f}', '123.456')
{'foo': 123.456}
  1. Including width and precision:
>>> trollsift.parse('{foo:5.2f}', '12.34')
{'foo': 12.34}

Width is considered minimum width, so that longer strings will also be matched (as long as the decimals match). This is to facilitate roundtrips with compose.

>>> trollsift.parse('{foo:5.2f}', '1234.56')
{'foo': 1234.56}
>>> trollsift.compose('{foo:5.2f}', {'foo': 1234.56})
'1234.56'

The following cases will not be matched because the string is too short or the number of decimals is incorrect:

>>> trollsift.parse('{foo:5.2f}', '1.23')
ValueError: String does not match pattern.
>>> trollsift.parse('{foo:5.2f}', '1.234')
ValueError: String does not match pattern.
  1. Zero/whitespace padding is also supported
>>> trollsift.parse('{foo:05.2f}', '01.23')
{'foo': 1.23}

@ghost
Copy link

ghost commented Nov 12, 2020

Congratulations 🎉. DeepCode analyzed your code in 3.316 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@sfinkens sfinkens assigned sfinkens and unassigned sfinkens Nov 12, 2020
@coveralls
Copy link

coveralls commented Nov 12, 2020

Coverage Status

Coverage increased (+0.5%) to 95.489% when pulling 3b60acb on sfinkens:fix-parsing-fixed-point into 144b2be on pytroll:master.

@djhoese
Copy link
Member

djhoese commented Nov 12, 2020

Parsed value is not a float but a string

Do you have an example of this bug?

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

I think this makes sense to me. I was worried it was going to get complicated and this definitely leans that way, but still not quite as bad as I thought it'd be. Nice job.

For your tests, what do you think about using pytest.mark.parameterize? You'd have to move the test function(s) outside the unittest class as regular functions or create a separate test class (ex. class TestParser:) that isn't based on unittest.TestCase.

@sfinkens
Copy link
Member Author

Do you have an example of this bug?

Sure, this is with the current master:

>>> trollsift.parse('{foo:f}', '12.34'}
{'foo': '12.34'}

For your tests, what do you think about using pytest.mark.parameterize?

Good idea! I'll update the tests.

@sfinkens
Copy link
Member Author

I also refactored the _convert method to reduce complexity (CodeFactor was complaining)

trollsift/parser.py Outdated Show resolved Hide resolved
Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

If tests pass then LGTM

Copy link
Member

@pnuu pnuu left a comment

Choose a reason for hiding this comment

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

LGTM

@mraspaud mraspaud merged commit 60e9eec into pytroll:master Nov 23, 2020
@sfinkens sfinkens deleted the fix-parsing-fixed-point branch November 23, 2020 15:37
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.

None yet

5 participants