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

deprecate XMonad.Util.Ungrab #843

Merged
merged 1 commit into from
Dec 9, 2023
Merged

deprecate XMonad.Util.Ungrab #843

merged 1 commit into from
Dec 9, 2023

Conversation

iogrt
Copy link

@iogrt iogrt commented Nov 16, 2023

Description

Deprecate XMonad.Util.Ungrab as it is going to be moved to XMonad.Operations in xmonad/xmonad#479

Checklist

  • I've read CONTRIBUTING.md

  • I've considered how to best test these changes (property, unit,
    manually, ...) and concluded: Tested manually with a config that uses unGrab and removed the XMonad.Util.Ungrab import.

  • I updated the CHANGES.md file

XMonad/Util/Ungrab.hs Outdated Show resolved Hide resolved
@@ -31,7 +31,6 @@ module XMonad.Config.Mate (
import XMonad
import XMonad.Config.Desktop
import XMonad.Util.Run (safeSpawn)
import XMonad.Util.Ungrab
Copy link
Member

Choose a reason for hiding this comment

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

This will result in a build failure related to this. I'm not sure it's worth bumping our bounds over, so we should probably be a bit more tame here as well

Copy link
Author

Choose a reason for hiding this comment

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

What I found is that if I don't remove this line, the compiler will complain about an ambiguous reference in the new version. Because unGrab will be defined both in XMonad.Operations (which is imported into this file from import XMonad) and XMonad.Util.Ungrab.

But thinking about it again. I could probably just do import XMonad hiding (unGrab). Would that be preferrable?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I was thinking about hiding the function from the XMonad import and perhaps adding a {-# OPTIONS_GHC -Wno-dodgy-imports #-} at the top of the file, to pacify the compiler when this is compiled against xmonad HEAD

Copy link
Member

@liskin liskin Dec 17, 2023

Choose a reason for hiding this comment

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

I'd be in favor of bumping xmonad and having #if MIN_VERSION_xmonad(0, 17, 9) in X.U.Ungrab so that users following the stable releases don't get the Ambiguous occurrence ‘unGrab’ error when compiling their configs, and just get a deprecaton warning.

Copy link
Member

Choose a reason for hiding this comment

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

(Note that for that to work, we can't have unGrab = Operations.unGrab but must reexport the import.)

Copy link
Member

Choose a reason for hiding this comment

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

I'd be in favor of bumping xmonad and having #if MIN_VERSION_xmonad(0, 17, 9) in X.U.Ungrab so that users following the stable releases don't get the Ambiguous occurrence ‘unGrab’ error when compiling their configs, and just get a deprecaton warning.

Right now users compiling against xmonad-0.17.2 should not get an error (as evidenced by the CI), since we're hiding (the possible non-existent) unGrab from the XMonad import. Still, bumping might not be such a bad idea

Copy link
Member

@liskin liskin Dec 17, 2023

Choose a reason for hiding this comment

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

I wasn't talking about compiling xmonad-contrib itself, but about compiling the xmonad.hs config. Anyone who imports both XMonad and XMonad.Util.Ungrab and uses unGrab will start getting errors. I mean, yeah, it is mentioned in "Breaking Changes", but I don't think it should actually be a Breaking Change when we can turn it into just a deprecation warning.

Copy link
Member

@liskin liskin Dec 17, 2023

Choose a reason for hiding this comment

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

(liskin/xmonad@5c6c3d4 & liskin@8526d93 are one way to solve this, if we want an ifdef)

Copy link
Member

@slotThe slotThe Dec 18, 2023

Choose a reason for hiding this comment

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

Oh, right, that might indeed be an issue. The ifdef sounds fine in that case; thanks for spotting this!

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I pushed the two commits then.

@slotThe slotThe merged commit 5c30cad into xmonad:master Dec 9, 2023
18 checks passed
@slotThe
Copy link
Member

slotThe commented Dec 9, 2023

Thanks!

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.

4 participants