-
Notifications
You must be signed in to change notification settings - Fork 710
Remove code duplication and unify definition of 'trim' function. #7795
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
Conversation
The CI failures seems to be caused by github upgrading to GHC 9.2.1. We'll have to fix our CI. |
Please kindly rebase and the two CI failures should be gone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice clean up, Thanks!
f8dec1b
to
57d6578
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Do we add edit: do we even do |
It seems we do:
Not sure about this particular case, because the function was available long before (though from different places). Anyway, it makes sense to add them ASAP, because we are on fire and the world is falling when we release. |
@Mikolaj |
Oh, I forgot not all people are relaxed regarding time travel. It will be 3.8. |
Attempt to fix #7744 .
That's my first ever cabal PR so I somehow don't know what I'm doing. Just picked up first
good-first-issue
because I've found this https://twitter.com/bkmlep/status/1432474203245015040Distribution.Utils.String
sounds like a good place fortrim
but I had to expose this module unfortunately.This the situation after my change:

Any advice or criticism fully welcomed!
Karol
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!