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

[refactor] pull1Port: use partition from Data.List #62

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

acl-cqc
Copy link
Collaborator

@acl-cqc acl-cqc commented Dec 3, 2024

Not quite a refactor - change fail to a BadPortPull error, and slight change to the error message for AmbiguousPortPull (now includes preceding but irrelevant/non-pulled ports)

pull1Port p = do
available <- get
case partition ((== p) . toPort . fst) available of
([], _) -> lift $ err $ BadPortPull $ "Port not found: " ++ p ++ " in " ++ showFn available
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not quite a refactor (1/2): changed fail to BadPortPull as the latter is otherwise unused

case partition ((== p) . toPort . fst) available of
([], _) -> lift $ err $ BadPortPull $ "Port not found: " ++ p ++ " in " ++ showFn available
([found], remaining) -> put remaining >> pure found
(_, _) -> lift $ err $ AmbiguousPortPull p (showFn available)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not quite a refactor (2/2): we used to pass to showFn here only the suffix of available beginning with the first match

Copy link
Collaborator

@croyzor croyzor left a comment

Choose a reason for hiding this comment

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

I'm not convinced this refactor is clearer than the original, save for the logic behind returning AmbiguousPortPull. The rest is kind of obscured by the extra use of the state monad

@acl-cqc acl-cqc force-pushed the acl/refactor_portpull2 branch from eb2795d to e2983c7 Compare December 4, 2024 09:48
@acl-cqc
Copy link
Collaborator Author

acl-cqc commented Dec 4, 2024

Ok, fair enough, I think the main improvement here is the use of partition which I think makes it much clearer what pull1Port is/was doing. I've reverted the second commit so gone back to just that (plus fail -> BadPortPull). How's that?

@acl-cqc acl-cqc requested a review from croyzor December 4, 2024 09:49
Copy link
Collaborator

@croyzor croyzor left a comment

Choose a reason for hiding this comment

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

Much better, thanks!

@acl-cqc acl-cqc changed the title Refactor pullPorts [refactor] pull1Port: use partition from Data.List Dec 4, 2024
@acl-cqc acl-cqc merged commit 5463a7f into main Dec 4, 2024
2 checks passed
@acl-cqc acl-cqc deleted the acl/refactor_portpull2 branch December 4, 2024 10:38
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.

2 participants