django-geo-spaas icon indicating copy to clipboard operation
django-geo-spaas copied to clipboard

get_or_create in nansat_ingestor should not use update_or_create

Open mortenwh opened this issue 4 years ago • 3 comments

In geospaas.nansat_ingestor.managers.get_or_create, you use Django's update_or_create method. This is confusing and should be changed. Perhaps adding a second manager method update_or_create? I think get_or_create is still needed, since a user doesn't necessarily want to update an existing entry.

mortenwh avatar Jul 24 '20 08:07 mortenwh

Thanks for comment, @mortenwh ! I agree that using update_or_create inside get_or_create is confusing. Maybe we should rather change the name geospaas.nansat_ingestor.managers.get_or_create to ingest or something similar to avoid confusion. Because in fact this is a very large method that extracts metadata from nansat object and saves into database. Ideally it should be split into more smaller methods.

But we have to use update_or_create inside - now there can be several sources of information for one record: one from django-geo-spaas-harvesting, another one (and more complete) from nansat ingestor.

akorosov avatar Aug 04 '20 08:08 akorosov

I see - so does it mean that it would be better to use the harvesting package for initial ingestion (e.g., before processing)?

On Tue, Aug 4, 2020 at 10:37 AM Anton Korosov [email protected] wrote:

Thanks for comment, @mortenwh https://github.com/mortenwh ! I agree that using update_or_create inside get_or_create is confusing. Maybe we should rather change the name geospaas.nansat_ingestor.managers.get_or_create to ingest or something similar to avoid confusion. Because in fact this is a very large method that extracts metadata from nansat object and saves into database. Ideally it should be split into more smaller methods.

But we have to use update_or_create inside - now there can be several sources of information for one record: one from django-geo-spaas-harvesting, another one (and more complete) from nansat ingestor.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nansencenter/django-geo-spaas/issues/127#issuecomment-668464152, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2UBICT4PFZPYYLGHGU3TR67CFBANCNFSM4PGQQ5EA .

-- Morten Wergeland Hansen, PhD Meteorologisk Institutt / Norwegian Meteorological Institute T.: (+47) 915 47 844

mortenwh avatar Aug 04 '20 11:08 mortenwh

Yes, you got it right. I'd rather keep this issue open to remind to refactor get_or_create some time later.

akorosov avatar Aug 10 '20 12:08 akorosov