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

Be able to turn off instances in password-instances #65

Open
Vlix opened this issue Feb 17, 2023 · 7 comments
Open

Be able to turn off instances in password-instances #65

Vlix opened this issue Feb 17, 2023 · 7 comments

Comments

@Vlix
Copy link
Collaborator

Vlix commented Feb 17, 2023

The same way you can turn off certain algorithms in password with the #63 PR, we could also have flags to turn of instances and with them dependencies of password-instances (since especially aeson and persistent can really bloat your dependency tree if you're not even using them)

@cdepillabout
Copy link
Owner

Related to #1.

Basically both would be ways to pick and choose exactly which instances / dependencies to use.

@Vlix
Copy link
Collaborator Author

Vlix commented Feb 17, 2023

Ah, you're right. Though I'd maybe rather do it in password-instances, just so as to not create more packages.
And creating a package for only 1 or 2 instances seems like a waste and doing that 3 times seems inefficient 🤔

@cdepillabout
Copy link
Owner

cdepillabout commented Feb 17, 2023

I think there are definitely situations where splitting up password-instances into multiple packages could be easier for the end-user (although like you say, not necessarily easier for us, the maintainers).

Here's an example. Let's say we decide to add flags to password-instances in order to enable/disable certain instances. We have a package called myapp, where we're directly depending on password-instances, with only the aeson instances enabled:

flowchart TD;
    myapp --> password-instances[password-instances with only aeson flag enabled];
Loading

Now, we want to depend on another library, which also has a dependency on password-instances. But it requires the persistent instances:

flowchart TD;
    myapp --> password-instances[password-instances with only aeson flag enabled];
    myapp --> some-library
    some-library --> password-instances;
Loading

Now, the end-user will get an compilation error when they try to build some-library, because it assumes password-instances will be built with the persistent flag enabled (but we don't have it enabled).

So the downside of not splitting up password-instances is that the end-user may have to be aware of what flags password-instances is built with, even in the case where they aren't directly depending on it! This isn't too bad for people using cabal or stack (since they give you easy ways of setting flags for transitive dependencies), but it a little more annoying when using distribution-supplied packages.

Splitting up password-instances into separate packages could make this easier for end-users, since you could directly depend on what you need, and not worry about what your transitive dependencies are doing. This should "just work":

flowchart TD;
    myapp --> password-instances-aeson;
    myapp --> some-library
    some-library --> password-instances-persistent;
Loading

@Vlix
Copy link
Collaborator Author

Vlix commented Feb 17, 2023

That is true, but like the current password flags, I'd argue for flags that have to be manually turned off. So that anyone who wants to specifically exclude a dependency, is able to do so.

If they then get a compilation error of the thing they turned off because of transient dependencies, they can just turn it back on, no? 🤔

@cdepillabout
Copy link
Owner

cdepillabout commented Feb 17, 2023

If they then get a compilation error of the thing they turned off because of transient dependencies, they can just turn it back on, no?

Either end users aren't aware of the flags (and they get really big dependency trees whenever they depend on password-instances), or they are aware of the flags, and have the possibility of getting into confusing situations when turning them off.

I'd argue that splitting password-instances into separate packages is much more "natural" to most Haskell developers, as opposed to flags determining what gets compiled in. It is easy to understand for end-users.

@Vlix
Copy link
Collaborator Author

Vlix commented Feb 17, 2023

Hmmmm, ok, that's a pretty good point, I guess.

Maybe we can add flags to the instances package as a last update, when we also "deprecate" it? Just to give power users more control if they're somehow stuck using password-instances over the newer ones? (though I hope the individual packages don't need very strict lower bounds 🤔 )

@cdepillabout
Copy link
Owner

cdepillabout commented Feb 18, 2023

@Vlix Oh, I guess I should be clear that I'm thinking we should split password-instances up into multiple packages (like password-instances-aeson, password-instances-persistent, etc 1), and then have a single password-instances package that depends on all the individual packages, just to give end-users an easy way to get started, in case they don't want to pick and choose. Although we would of course encourage library authors to depend on the individual password-instances-* packages.

So in that setup, I agree that potentially having flags in password-instances would be fine, and it may be helpful for power users.

Footnotes

  1. In split password-instances into multiple packages #1, you suggested that the package names should instead be password-aeson, password-persistent, etc. Those names might make more sense, especially if those packages contain helper functions, but they might not be as "SEO-friendly", since the foobar-instances naming convention is often used in Haskell.

@Vlix Vlix changed the title Be able to turn of instances in password-instances Be able to turn off instances in password-instances Jul 12, 2023
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

2 participants