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

The modules in semialign have a lot of overlapping exports #198

Open
mitchellwrosen opened this issue May 30, 2024 · 4 comments
Open

The modules in semialign have a lot of overlapping exports #198

mitchellwrosen opened this issue May 30, 2024 · 4 comments

Comments

@mitchellwrosen
Copy link

Hi there,

I've noticed that the semialign package exports many symbols from more than one module.

These symbols are exported by both Data.Align and Data.Semialign:

  • Align(..)
  • Unalign(..)
  • alignVectorWith
  • lpadZip
  • lpadZipWith
  • padZip
  • padZipWith
  • rpadZip
  • rpadZipWith
  • salign

These symbols are exported by both Data.Semialign and Data.Zip:

  • Unzip(..)
  • Zip(..)
  • unzipDefault

And finally, these symbols are exported by Data.Align, Data.Semialign, and Data.Zip:

  • Semialign(..)

I would not have ever noticed maybe 5 years ago, but now HLS is robust enough to recommend imports when a symbol is out of scope. So, I think it would be preferred if each symbol was only exported by one module.

@phadej
Copy link
Collaborator

phadej commented May 31, 2024

In the beginning there was only Data.Align with only Align type-class. Then hierarchy was refactored and all things were put into Data.Semialign, but the Data.Align is there for backward-compat (and Data.Zip for consistency).

I'm not really keen to just remove exports, as that would be breaking change for no reason. If there will be another breaking change, than maybe then would make sense to bundle.


So, I think it would be preferred if each symbol was only exported by one module.

Say that to base (and ghc-internal) ;) or about any package having exposed internal modules.

@mitchellwrosen
Copy link
Author

Sure, I agree with the backwards compatibility concerns. The problem is certainly exacerbated by HLS providing a plethora of similar options.

Well, this is where we're at today (just to put a visual on the ticket):

2024-05-31-100621_789x436_scrot

@phadej
Copy link
Collaborator

phadej commented May 31, 2024

The way HLS suggests four options per module is HLS's issue.

@rickowens
Copy link

rickowens commented Jul 16, 2024

as that would be breaking change for no reason

I would like to politely object to the "no reason" characterization. I believe cleaning up the API is a noble cause, highly valuable for its own sake! As always, this must be weight against all factors, but IMO a clean API is significantly more important than "no reason".

edit: I am also aware that the problem is widespread, including in base. base also uses String everywhere when modern libraries use Text :-)

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

No branches or pull requests

3 participants