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-11162: Replace all use of Coord and subclasses with SpherePoint #51

Merged
merged 2 commits into from Mar 23, 2018

Conversation

r-owen
Copy link
Contributor

@r-owen r-owen commented Mar 16, 2018

No description provided.

@@ -48,9 +49,9 @@ struct UnitSystem {
* set such that unit flux is the given magnitude. See @ref modelfitUnits for an explanation
* of why we frequently use this system.
*/
UnitSystem(afw::coord::IcrsCoord const & position, std::shared_ptr<const lsst::afw::image::Calib> calibIn,
UnitSystem(afw::geom::SpherePoint const& position, std::shared_ptr<const lsst::afw::image::Calib> calibIn,

Choose a reason for hiding this comment

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

No need to move the &. Same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clang-format; I can make it a separate commit if you want

Choose a reason for hiding this comment

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

My concern was for consistency in the file/package. Did it flag the other cases in this file?

Copy link
Contributor Author

@r-owen r-owen Mar 22, 2018

Choose a reason for hiding this comment

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

I can run clang-format on the whole file if you prefer. Normally I only run it on code I change.

Note that the output of clang-format is always acceptable; we'd go mad otherwise.

Choose a reason for hiding this comment

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

I prefer to keep the same style within a file, but I'm happy to be overruled if this is not what others want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I'll run clang-format as a separate commit.

Copy link

@rearmstr rearmstr left a comment

Choose a reason for hiding this comment

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

Looks good. Only minor changes.

as per reviewer request for internal consistency
in the placement of & and a desire to use clang-format
to format my new code.
@r-owen r-owen merged commit 7739892 into master Mar 23, 2018
@ktlim ktlim deleted the tickets/DM-11162 branch August 25, 2018 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants