search

ckan / ckanext-dcat

CKAN ♥ DCAT
https://docs.ckan.org/projects/ckanext-dcat
165 stars 145 forks source link

Make _object_value_int more robust by accepting decimals as well #133

Closed seitenbau-govdata closed 5 years ago

seitenbau-govdata commented 6 years ago

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.

https://github.com/GovDataOfficial/ckanext-dcatde/issues/6

metaodi commented 6 years ago

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 commented 6 years ago

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:

seitenbau-govdata commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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

metaodi commented 5 years ago

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

seitenbau-govdata commented 5 years ago

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

amercader commented 5 years ago

Thanks @seitenbau-govdata and @metaodi

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

metaodi commented 5 years ago

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

amercader commented 5 years ago

@metaodi fantastic thanks! You should now have write permissions :sunglasses: