Skip to content

Conversation

@blackheaven
Copy link
Contributor

A bit of documentation, let me know if you had something else in mind.

@blackheaven
Copy link
Contributor Author

I don't think the CI failure is related to my PR.

@Vlix
Copy link
Collaborator

Vlix commented Oct 7, 2025

Ah, yes. optparse-applicative has a newer version we can probably allow.

Not sure if I like the repetition of the Testing segment in all the modules. But then again, it's good information close to the usage site of the library.
I might have to think about it a bit more.

@Vlix
Copy link
Collaborator

Vlix commented Oct 7, 2025

Ah, nevermind. I've literally asked for that in the #83 issue. 🤦

I would like to also see the second part of the checkboxes of the issue's first post ticked in this PR:
Mostly changing the "SOME-HASH" sections in the password-cli/README to 'SOME-HASH' and adding a comment that using " might interpolate in some cases (e.g. when a hash or password has a $ in it)

@blackheaven
Copy link
Contributor Author

Ah, nevermind. I've literally asked for that in the #83 issue. 🤦

I would like to also see the second part of the checkboxes of the issue's first post ticked in this PR: Mostly changing the "SOME-HASH" sections in the password-cli/README to 'SOME-HASH' and adding a comment that using " might interpolate in some cases (e.g. when a hash or password has a $ in it)

Done :)

@blackheaven
Copy link
Contributor Author

@Vlix can you have another look please?

You can use [password-cli](https://github.com/cdepillabout/password/tree/master/password-cli) to test it:
> $ password-cli check argon2 --hash "SOME-HASH"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These kind of examples might also be better with single quotes '.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, I have fixed it and the other ones

@blackheaven
Copy link
Contributor Author

@Vlix thanks for you feedback, ready for a new review.

Copy link
Collaborator

@Vlix Vlix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready to be merged :)

I'll hold off on actually merging until publishing the password-cli to hackage. Don't want to reference it in documentation when it's not actually available.

@Vlix Vlix mentioned this pull request Oct 12, 2025
22 tasks
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

Successfully merging this pull request may close these issues.

2 participants