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

Save indicator as GeoJSON Feature to DB #149

Merged
merged 4 commits into from
Aug 30, 2021

Conversation

matthiasschaub
Copy link
Collaborator

@matthiasschaub matthiasschaub commented Aug 18, 2021

Description

Extend database schema with one additional attribute of type JSON.
During saving indicator attribute to database, dump the
indicator.as_feature() output to the database as well. The reason is to
not lose any data when saving and loading and indicator from and to the
database such as the indicator.data attribute (see issue #145).

Workflow to save and load an indicator form the database will stay the same.
This work will be helpful in solving issue #145.

Checklist

  • I have updated my branch to main (e.g. through git rebase main)
  • My code follows the style guide and was checked with pre-commit before committing
  • I have commented my code
  • I have added sufficient unit and integration tests
  • I have updated the CHANGELOG.md

@matthiasschaub matthiasschaub force-pushed the db-save-results-as-geojson branch 4 times, most recently from 0e8966c to c9326ce Compare August 20, 2021 14:07
@matthiasschaub matthiasschaub changed the title WIP Save indicator as GeoJSON Feature to DB Aug 20, 2021
@matthiasschaub matthiasschaub marked this pull request as ready for review August 23, 2021 12:00
Copy link
Member

@joker234 joker234 left a comment

Choose a reason for hiding this comment

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

If you run the tests, you're getting this error:

TypeError: Object of type datetime is not JSON serializable

This means that the JSON library is not capable of serializing datetime objects as part of a JSON. I would suggest to either represent the datetime objects beforehand in another date format (e.g. unix timestamp) or use a default serializer (e.g. str). Since you're not doing the serializing by yourself, I would suggest to find any representation and follow the first suggestion.

I made a suggestion, how you could fix it. You're free to use this and merge the other branch in yours or find something else: db-save-results-as-geojson...db-save-results-as-geojson-proposed-fix

ToDo: add proper tests (see your checklist)

@matthiasschaub
Copy link
Collaborator Author

matthiasschaub commented Aug 26, 2021

Great catch! Thank you for looking into that.

We had this problem in the CLI module as well and have a solution already in place.

# helper.py
def datetime_to_isostring_timestamp(time: datetime) -> str:
    """
    Checks for datetime objects and converts them to ISO 8601 format.

    Serves as function that gets called for objects that can’t otherwise be
    serialized by the `json` module.
    It should return a JSON encodable version of the object or raise a TypeError.
    https://docs.python.org/3/library/json.html#basic-usage
    """
    try:
        return time.isoformat()
    except AttributeError:
        raise TypeError

I pushed a fixed version.

joker234
joker234 previously approved these changes Aug 27, 2021
Extend database schema with one additional attribute of type JSON.
During saving indicator attribute to database, dump the
indicator.as_feature() output to the database as well. The reason is to
not lose any data when saving and loading and indicator from and to the
database such as the indicator.data attribute (see issue #145).

Workflow to save and load an indicator form the database will stay the same.
This work will be helpful in solving issue #145.
Use default parameter of json.dumps to define that datetime should be converted to iso time string
@matthiasschaub matthiasschaub merged commit 6b411bc into main Aug 30, 2021
@matthiasschaub matthiasschaub deleted the db-save-results-as-geojson branch August 30, 2021 09:48
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

2 participants