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

Make _object_value_int more robust by accepting decimals as well #133

Merged

Conversation

seitenbau-govdata
Copy link
Member

@seitenbau-govdata seitenbau-govdata commented Aug 29, 2018

Parsing values containing dots led to value loss in _object_value_int.

This also parses floating point values to integers, which makes parsing more robust.

GovDataOfficial/ckanext-dcatde#6

@metaodi
Copy link
Member

metaodi commented Sep 13, 2018

What is the use case of this? This might lead to unexpected behavior for users, since values are not rounded, i.e. the literal "3.8" will be saved as the integer 3. As far as I can see this method is currently used for the byteSize. I'm not entirely sure if it's better to silently lose the value (since it's not a valid integer) or to just "cut" it, to make it valid.

@seitenbau-govdata
Copy link
Member Author

Thank's for your feedback, @metaodi .

Some of our harvested datasets contain values like 16.0 in byteSize.
As looking for a .0 is more complex than simply calling float, we decided to implement it like that.

You are right, results could be unexpected for users. But silently loosing the value is worse in our opinion.

So we suggest one of those options:

  • round the value
  • cut the value (as implemented right now)
  • when the value is a float, only keep it if it ends with .0

@seitenbau-govdata
Copy link
Member Author

@metaodi Please let me know what your preferred choice for the implementation is. If you think it's completely not wanted we can also close the pull request.

@metaodi
Copy link
Member

metaodi commented Oct 30, 2018

@seitenbau-govdata I think it's fine like it is, cutting is better than losing. And rounding the value might not be expected as well.

@seitenbau-govdata
Copy link
Member Author

@metaodi Ok, great. Who is responsible merging the PR? You or @amercader ? Of course, I'm always interested in @amercader opinion. :-)

@metaodi
Copy link
Member

metaodi commented Nov 7, 2018

@seitenbau-govdata I'm not (yet 😉 ) a maintainer of this repo. So @amercader is responsible 😊

@seitenbau-govdata
Copy link
Member Author

Ah, ok. Then I confused it with the repo https://github.com/ckan/ckanext-harvest. 😊 But there your'e a maintainer.

@amercader amercader merged commit c098d03 into ckan:master Nov 8, 2018
@amercader
Copy link
Member

Thanks @seitenbau-govdata and @metaodi

@metaodi I can always use a pair of hands, would you like to be a maintainer?

@metaodi
Copy link
Member

metaodi commented Nov 8, 2018

@amercader sure, I'd like to help and I'm anyway reviewing PRs

@amercader
Copy link
Member

@metaodi fantastic thanks! You should now have write permissions 😎

@seitenbau-govdata seitenbau-govdata deleted the more-robust-parsing-object_value_int branch December 14, 2018 13:51
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.

3 participants