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

Do not export the constructor of Pass #5

Closed
Vlix opened this issue Jan 21, 2020 · 3 comments · Fixed by #6
Closed

Do not export the constructor of Pass #5

Vlix opened this issue Jan 21, 2020 · 3 comments · Fixed by #6
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@Vlix
Copy link
Collaborator

Vlix commented Jan 21, 2020

To make it more safe, it'd be better that the Pass data constructor not be exported. This way, users can't pattern match on it and just remove the Text without using a function that's clearly labeled as unsafe.

Obviously, accompanying this should be a function like mkPass :: Text -> Pass to have users still be able to produce a Pass.

@cdepillabout cdepillabout added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Jan 21, 2020
@cdepillabout
Copy link
Owner

This seems like a good idea.

Maybe in the future, we could export the constructor in a module like Data.Password.Internal, but for now just hiding the constructor seems like a good idea.

@Vlix
Copy link
Collaborator Author

Vlix commented Jan 22, 2020

Also, I'm wondering what the added value is of having a Read instance for Pass. If it has an IsString instance, you can just fromString foo where foo :: String. Any read function is also from a String, so it's superfluous AND because it makes more sense to implement a Show function (see below), you'd rather not have a Read and Show that are not isomorphic.

Defining the Show instance as follows:

instance Show Pass where
  show _ = "**PASSWORD**"

This way no one can make their own Show function for Pass (maybe without thinking it through, just to make something type check) and forcing the user, again, to use a function labeled unsafe to get to print the password.

@cdepillabout
Copy link
Owner

I am kind of on the fence with having a Show instance for Pass.

On one hand, I don't want people to ever get lax about just showing a Pass. I'd like users to explicitly have to use the unsafe functions if they want to go from Pass to String (even if show doesn't actually show the password).

On the other hand, it is really annoying when types don't implement a Show instance. It becomes very slightly harder to implement a Show instance for any record containing a Pass.

I guess I'd be okay with adding a Show instance as long as there is a warning in the documentation about exactly what it does.


you'd rather not have a Read and Show that are not isomorphic

I don't really buy into the Read / Show isomorphism law. Especially in cases like this where it could make sense to keep them different.

Now that I've taken a look at it, I think the bigger problem is that there is no documentation about what format the Read function is expecting. And it is hard to figure out because there is also no show instance.


To sum up, I guess I don't really care about Show and Read, but if they do exist, they should have good documentation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants