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

Move Heading and conversions to Location module #1103

Merged
merged 5 commits into from
Feb 13, 2023
Merged

Conversation

xsebek
Copy link
Member

@xsebek xsebek commented Feb 11, 2023

@xsebek xsebek requested review from kostmo and byorgey February 11, 2023 12:15
@xsebek xsebek mentioned this pull request Feb 11, 2023
DAbsolute e -> cardinal $ toHeading e
where
cardinal = const
relative = id
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of defining relative = id? Why not just have DLeft -> perp and so on? (Maybe this code was already like that but I didn't notice before.)

Copy link
Member Author

Choose a reason for hiding this comment

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

No good reason, I think we originally used it for the absolute/relative distinction, but that has disappeared with the direction constructors.


cardinal = DirInfo directionSyntax . const
relative = DirInfo directionSyntax
-- | Direction name is generate from Direction data constuctor
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-- | Direction name is generate from Direction data constuctor
-- | Direction name is generated from Direction data constuctor

applyTurn d = case d of
DRelative e -> case e of
DLeft -> relative perp
DRight -> relative (fmap negate . perp)
Copy link
Member

Choose a reason for hiding this comment

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

I've been meaning to replace fmap negate with just negated

Copy link
Member Author

Choose a reason for hiding this comment

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

Neat, thanks! 👍

@xsebek xsebek added the merge me Trigger the merge process of the Pull request. label Feb 13, 2023
@mergify mergify bot merged commit 42abf59 into main Feb 13, 2023
@mergify mergify bot deleted the direction-split branch February 13, 2023 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants