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

Issue126 remove geospaas urls #153

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

azamifard
Copy link
Contributor

removing geospaas.urls.py as mentioned in #126

@azamifard azamifard marked this pull request as ready for review March 3, 2021 13:54
@azamifard
Copy link
Contributor Author

@akorosov @aperrin66 I delete the whole nansat_ingestor app in b90668a because we have developed localharvester in the harvesting repo! :)

Copy link
Member

@aperrin66 aperrin66 left a comment

Choose a reason for hiding this comment

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

  • Removing nansat_ingestor should be in a different pull request.
  • There are still references to geospaas.urls in the repository

Comment on lines -179 to +178
gg, created = GeographicLocation.objects.get_or_create(geometry=WKTReader().read(geom_wkt))
gg, created = GeographicLocation.objects.get_or_create(geometry=WKTReader().read(geom_wkt))
Copy link
Member

Choose a reason for hiding this comment

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

If the error due to max precision does not happen anymore, this test can probably be deleted.

Or maybe it still happens with a particular database backend?
Even if it is the case, I am not sure if we should keep that test, since it does not validate a desired behavior.
Maybe it would be more relevant to create an issue about this problem, describing steps to reproduce it.

Any thoughts about that @akorosov, @mortenwh ?

Copy link
Member

@akorosov akorosov Mar 8, 2021

Choose a reason for hiding this comment

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

I haven't seen this error myself. However the test can remain here.
What's needed:

  • rename the test to reflect that not_null_constraint_failed is not reproduced anymore
  • add a relevant comment that db actually can handle high precision numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 04899cf

@azamifard
Copy link
Contributor Author

azamifard commented Mar 4, 2021

@aperrin66 ok! by the last four commits I dont do anything regarding nansat_ingestor here. I will do it in another PR.

@azamifard
Copy link
Contributor Author

  • geospaas.urls

which references do you mean @aperrin66 ?

@azamifard
Copy link
Contributor Author

in geospaas.rst file?

@aperrin66
Copy link
Member

  • geospaas.urls

which references do you mean @aperrin66 ?

in geospaas.rst file?

Among others, you can just look for the "geospaas.urls" string in django-geo-spaas to find them.

@azamifard azamifard requested a review from aperrin66 March 4, 2021 08:10
Copy link
Member

@akorosov akorosov left a comment

Choose a reason for hiding this comment

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

Minor correction to the test__reproduce__not_null_constraint_failed() test

@azamifard azamifard requested a review from akorosov March 9, 2021 14:30
@aperrin66 aperrin66 linked an issue Dec 27, 2021 that may be closed by this pull request
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.

The geospaas.urls module is unusable
3 participants