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

Fixed #25585 -- Allowed None to be set as SRID and SRS value for OGRGeometry. #5458

Closed
wants to merge 1 commit into from

Conversation

sir-sigurd
Copy link
Member

@@ -320,6 +320,11 @@ def test_srs(self):
self.assertEqual('WGS 72', ring.srs.name)
self.assertEqual(4322, ring.srid)

# test assigning of SRS and SRID to None
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit, but I prefer stating the expected behavior like this: # SRS and SRID may be assigned to None. since all tests are obviously for testing things.

@timgraham timgraham changed the title Fixed #25585 -- Allowed None to be set as SRID and SRS value for `O… Fixed #25585 -- Allowed None to be set as SRID and SRS value for OGRGeometry. Oct 21, 2015
# test assigning of SRS and SRID to their own values
mpoly = OGRGeometry(mp.wkt, srs=None)
mpoly.srs = mpoly.srs
mpoly.srid = mpoly.srid
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we simply test setting the srs and srid to None? Is there something specific about using their own values?

Copy link
Member Author

Choose a reason for hiding this comment

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

I find it more expressive. It's obvious that attribute should allow assigning to its own value, unlike that attribute should allow assigning to None.

Copy link
Member

Choose a reason for hiding this comment

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

OK, then the comment might be completed by "even when srs is None".

@timgraham
Copy link
Member

Comment updated and merged in 0dbe897, thanks!

@timgraham timgraham closed this Oct 21, 2015
@sir-sigurd sir-sigurd deleted the allow-none-for-srid branch October 22, 2015 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants