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

DM-10765: Replace existing WCS classes with SkyWcs #40

Merged
merged 4 commits into from Feb 16, 2018
Merged

Conversation

r-owen
Copy link
Contributor

@r-owen r-owen commented Dec 12, 2017

No description provided.

@@ -30,7 +30,8 @@
import sys

import lsst.daf.base as dafBase
import lsst.afw.image as afwImage
from lsst.afw.fits import readMetadata
from lsst.afw.geom import makeSkyWcs
import lsst.skypix as skypix
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's no specific reason not to, I do prefer import ... as ... (as opposed to from ... import ...) so that it is more obvious where the function comes from when used far away from the import statements. Ditto for all other instances in other packages...

Copy link
Contributor Author

@r-owen r-owen Jan 9, 2018

Choose a reason for hiding this comment

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

I hear you and I used to feel that way myself. However, I now strongly prefer to import symbols when only one or two are used, especially if I the names are unambiguous. This allows me to avoid the controversial use of namespace abbreviations or avoid spelling out the full library name prefix every time the symbol is used. If namespace abbreviations were not controversial I'd be happier to use them. But even that adds characters that can obscure the code, and spelling out the full namespace is worse in that way and often results in extra line wraps.

I think this partly comes from getting used to how C++ handles namespaces. In any case it's clearly a matter of taste, and since both are allowed, I'm afraid I'm going to continue importing the symbols. I agree it can get out of hand (afw test_skyWcs.py, alas) and you should feel free to complain if you find instances that have.

@@ -26,6 +26,7 @@
import unittest

import lsst.afw.image
from lsst.afw.geom import SkyWcs
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@r-owen r-owen merged commit b92a491 into master Feb 16, 2018
@ktlim ktlim deleted the tickets/DM-10765 branch August 25, 2018 06:16
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