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-15139: Rename invert() and getInverse() to inverted() #49

Merged
merged 3 commits into from Aug 12, 2018

Conversation

r-owen
Copy link
Contributor

@r-owen r-owen commented Aug 10, 2018

No description provided.

Improve some documentation copied from AST implied that
`FrameSet.getInverse` inverted the frame set in place.
@r-owen r-owen changed the title DM-15139: Rename inverse() and getInverse() to inverted() DM-15139: Rename invert() and getInverse() to inverted() Aug 10, 2018
@@ -90,7 +90,7 @@ class Mapping : public Object {
Is this an inverted mapping?

Note: this gets the @ref Mapping_Invert "Invert" attribute.
This method is not called `getInvert` because that is too similar to @ref getInverse.
This method is not called `getInvert` because that is too similar to @ref inverted.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this comment makes sense anymore: isInverted is closer to inverted than getInvert would be. I don't think it's worth changing the API (ie, renaming isInverted to getInvert), although if you were keen to do that I wouldn't object, but I do think we should remove the comment.

Copy link
Contributor Author

@r-owen r-owen Aug 12, 2018

Choose a reason for hiding this comment

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

I think it important to say why it is not called getInvert because most of astshim has getX for every attribute X. However, I will change it to say "because that sounds like it might return the inverse". That is a bit more accurate than my original comment.

@r-owen r-owen merged commit 0febb12 into master Aug 12, 2018
@ktlim ktlim deleted the tickets/DM-15139 branch August 25, 2018 04:31
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