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

Added package flags for optional instances #111

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

dmcclean
Copy link
Collaborator

@dmcclean dmcclean commented Dec 2, 2015

As the next step in the tentative plan at #95 on how to deal with various desirable instances which aren't worth the dependencies they would incur to all users, I've prepared this sketch of how it will work.

Comments are welcome on whether this is the right road or not.

It's not ready yet for a few reasons:

  • Documentation in the readme
  • Instances for the linear package
  • Instances for the aeson package
  • Instances for the serialization classes for various ancillary types
  • Changes to the travis build to add the following flavors (only against the latest GHC, to avoid creating an enormous matrix)
    • With no flags
    • With each flag alone
    • With all flags

@dmcclean
Copy link
Collaborator Author

OK, this is now merge worthy.

The build with all flags enabled is currently allowed to fail in the travis file. This is because of version conflicts on hackage. Addressing #127 would probably be the best way to avoid problems of this kind for the long term.

The only remaining question I have on whether to merge this is whether it is a good idea or not.

@dmcclean dmcclean added this to the Version 1.1 milestone Dec 29, 2015
@dmcclean
Copy link
Collaborator Author

This is up for review until 8 January 2016.

@dmcclean
Copy link
Collaborator Author

dmcclean commented Jan 1, 2016

See this cabal issue for some discussion of why this may not be the way to go: haskell/cabal#2821

@bjornbm
Copy link
Owner

bjornbm commented Jan 6, 2016

It's a hairy problem, these orphan instances. Does merging this remove any current functionality/instances, or only allow bringing in additional functionality via flags? (Sorry for not reviewing in detail.)

@bjornbm
Copy link
Owner

bjornbm commented Jan 6, 2016

I'm not a huge fan of the Functor one as, in contrast to the others, it is not a matter of dependencies. Choosing whether or not to have a Functor instance att package installation time seems heavy-handed. Can you remind me of the use case for this instance (versus users defining a newtype if they need a Functor instance and don't want an orphan)? Was it for use with linear?

@dmcclean
Copy link
Collaborator Author

You are absolutely right about that. It was for use with linear, but it isn't worth it. All my actual code that uses this uses vector-space instead for dimensional vectors, and only uses linear for quaternions.

I'm going to amend this by getting rid of the functor and linear bits. The others only add functionality. Still not sure if they are a good idea, monitoring a discussion here about this: haskell/cabal#2821.

@bjornbm
Copy link
Owner

bjornbm commented Jan 19, 2016

I have found myself implementing an AEq instance a few times so that would be a candidate for similar treatment to the ones in this branch once we (you) decide what to do. (I'm not implying that you should add it, rather that there is a good chance that I will sooner or later.)

I suppose merging this should cause no problem except adding complexity (which certainly can be a concern in and of itself) if the flags are off by default.

Or should they be on by default on the assumption that stack will magically make the dependencies "just work" for the majority of use(r)s, and the flags are there as a fallback if needed to resolve pathological dependency graphs?

@dmcclean
Copy link
Collaborator Author

There's a comment on that thread by Edward suggesting (I think, I'm a little confused by it frankly) that he prefers the on-by-default approach. The on-by-default approach is more stack-friendly, and still would allow power users to make a minimal build for some kind of embedded use.

The AEq instance is a good idea. Arbitrary, too. We should make issues to track those.

@bjornbm
Copy link
Owner

bjornbm commented Jan 22, 2016

Ah, yes, definitely Arbitrary.

And I also use PlotValues from chart but if I recall that one is a bit tricky. I'll have to double-check my implementation to see if it is solid. It probably pulls in a lot of dependencies though which may make it not worth it.

@dmcclean
Copy link
Collaborator Author

Perhaps instead some kind of wrapper library around chart, to force you to pick units and then help you tack their names on to the end of your axis labels. Direct support for charting quantities seems like it might be problematic because it implicitly uses siUnit to treat quantities as if they were raw numbers? OTOH if we had #110 sorted, plotting that would seem safe.

@bjornbm
Copy link
Owner

bjornbm commented Jan 22, 2016

Yes, I realised/remembered shortly after my reply that it's not so simple in general. Although SI unit is a reasonable choice for the quick and dirty stuff, we probably need a newtype wrapper or something to allow picking a unit.

@bjornbm
Copy link
Owner

bjornbm commented Jan 22, 2016

And thus it can go in another package without causing orphans.

On Fri, Jan 22, 2016 at 11:01 AM, Douglas McClean
[email protected] wrote:

Perhaps instead some kind of wrapper library around chart, to force you to pick units and then help you tack their names on to the end of your axis labels. Direct support for charting quantities seems like it might be problematic because it implicitly uses siUnit to treat quantities as if they were raw numbers? OTOH if we had #110 sorted, plotting that would seem safe.

Reply to this email directly or view it on GitHub:
#111 (comment)

This was referenced Jan 22, 2016
@LaurentRDC
Copy link
Collaborator

Chiming in here to say that I would love for this feature to land, especially the Binary instances.

As a motivation, the Cloud Haskell implementation distributed-process requires types to be instances of Binary to be sent between cloud nodes.

Is there anything I can help with here?

@bjornbm
Copy link
Owner

bjornbm commented Aug 19, 2024

Good question. As you can tell this stalled and frankly I'm not sure why. Do you recall, @dmcclean?

@LaurentRDC you are welcome to try to sort it out, do a manual merge, and test it. I will admit I haven't followed how/if these packages and their APIs have evolved in the past 8 years (and I guess we don't want to be stuck at LTS-5 dependencies 😆).

bjornbm added a commit that referenced this pull request Aug 21, 2024
May revert to wording from #111 if we feel it is more relevant if/when more flags added.
@bjornbm
Copy link
Owner

bjornbm commented Aug 21, 2024

Binary instance flags added in #224. The rest have not been added but at least now there is a “template” for further flags in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants