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

Fix Monoid instances for ForeignLib & Executable #9555

Merged
merged 1 commit into from
Jan 13, 2024

Conversation

sheaf
Copy link
Collaborator

@sheaf sheaf commented Dec 22, 2023

The Semigroup and Monoid instances for ForeignLib were completely broken: for the foreignLibVersionInfo and foreignLibVersionInfo, we essentially had the following:

mempty :: Maybe XYZ
mempty = Nothing

(<>) :: Maybe XYZ -> Maybe XYZ -> Maybe XYZ
_ <> b = b

which is obviously not a valid Monoid, as Just x <> Nothing = Nothing, violating the identity law.

The Semigroup instance for Executable was also deeply suspicious, as it combined the module paths, which makes no sense. Now we instead error if the two module paths are different (and both nonempty).

Copy link
Collaborator

@ffaf1 ffaf1 left a comment

Choose a reason for hiding this comment

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

Well spotted, target monoids have many quirks.

@alt-romes
Copy link
Collaborator

@mergify rebase

Copy link
Contributor

mergify bot commented Jan 9, 2024

rebase

❌ Unable to rebase: user alt-romes is unknown.

Please make sure alt-romes has logged in Mergify dashboard.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 9, 2024

@mergify rebase

Copy link
Contributor

mergify bot commented Jan 9, 2024

rebase

✅ Branch has been successfully rebased

@Mikolaj
Copy link
Member

Mikolaj commented Jan 9, 2024

@alt-romes: apparently the Write permissions are not enough for rebase. I've bumped yours so next time it should work. Thank you for rescuing this PR lost in time.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 9, 2024

Hmm, something is still borked.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 9, 2024

@mergify refresh

Copy link
Contributor

mergify bot commented Jan 9, 2024

refresh

✅ Pull request refreshed

@Mikolaj
Copy link
Member

Mikolaj commented Jan 9, 2024

github Actions are having problems: "Error: Internal server error occurred while resolving "actions/cache@v3". Internal server error occurred while resolving "actions/checkout@v4". Internal server error occurred while resolving "actions/upload-artifact@v3". Internal server error occurred while resolving "haskell-actions/setup@v2""

Let's just wait it out.

@alt-romes alt-romes added the merge me Tell Mergify Bot to merge label Jan 10, 2024
@alt-romes
Copy link
Collaborator

@mergify rebase

Copy link
Contributor

mergify bot commented Jan 10, 2024

rebase

❌ Unable to rebase: user alt-romes is unknown.

Please make sure alt-romes has logged in Mergify dashboard.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 10, 2024

@mergify rebase

Copy link
Contributor

mergify bot commented Jan 10, 2024

rebase

✅ Branch has been successfully rebased

@Mikolaj
Copy link
Member

Mikolaj commented Jan 11, 2024

@mergify rebase

Copy link
Contributor

mergify bot commented Jan 11, 2024

rebase

✅ Branch has been successfully rebased

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 13, 2024
The Semigroup and Monoid instances for ForeignLib were completely
broken: for the `foreignLibVersionInfo` and `foreignLibVersionInfo`,
we essentially had the following:

  mempty :: Maybe XYZ
  mempty = Nothing

  (<>) :: Maybe XYZ -> Maybe XYZ -> Maybe XYZ
  _ <> b = b

which is obviously not a valid Monoid, as `Just x <> Nothing = Nothing`,
violating the identity law.

The Semigroup instance for Executable was also deeply suspicious, as
it combined the module paths, which makes no sense. Now we instead error
if the two module paths are different (and both nonempty).
@mergify mergify bot merged commit f01e000 into haskell:master Jan 13, 2024
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants