Open Vlix opened 3 years ago
Thanks for the feedback. There's an example in the readme. It's a literate haskell file that can be compiled and run.
The field naming of Config
is just how we do record field naming at Obsidian. Same with the constructor naming. It's something we're consistent about across all* of our libraries.
I agree about limiting the exports. I also agree that there should be an exported function with the type signature (MonadLogger m, MonadIO m) => Config -> Text -> m Result
so that people can use the functionality without running MonadPwned. If that function were present, both modes of operation would be made available by the library. I'm not sure I see the value of eliminating the MonadPwned stuff if one is able to easily opt out of it.
* At least, that's what we aim for.
I don't see you put underscores in front of record fields in your other packages, though (at least the few I skimmed through).
Having a minimal example in the module itself is benificial for users who look up the module via hackage.
If you really don't want to include a minimum example of the haveIBeenPwned
function in the module itself, at least mention that there's an example in the README of the package.
I don't see how MonadPwned
does anything, really. Do you expect other users to implement their own way of doing exactly what this library does? Any user of this library would only have to make a PwnedT
just to use the haveIBeenPwned
function.
Given a user already has a Reader with their settings, this is what the usage of this library would look like now:
...
cfg <- asks hibpConfig
result <- haveIBeenPwned pwd `runPwnedT` cfg
...
And if MonadPwned
and PwnedT
is removed from the library:
...
cfg <- asks hibpConfig
result <- haveIBeenPwned cfg pwd
...
If you want to keep it while also exporting a function Config -> Text -> m Result
, that sounds to me like you use this package in your code somewhere and you just dropped some plumbing you use in this library, even though it's superfluous to other users of this library.
I also noticed a few more things:
data-default
is a dependency of the library, but not used at allmtl
can be removed completely if the whole ReaderT
stuff is removedsafe
is only used for its readMay
function, this could also be removed by using Text.Read.readMaybe
So that's 3 more dependecies that could be dropped.
The readme is also on hackage. We prefer to put example code in literate haskell source files to ensure that the example stays up-to-date as the code changes. Something like doctest
could also provide a similar guarantee, if you'd like to take a crack at adding some documentation inline.
I agree that dependencies we no longer use should be removed from the cabal file. That's just an oversight caused by an absence of built-in warnings about that sort of thing.
There are other possible instances of MonadPwned. You can, for instance, download the entire database of breached passwords from the haveibeenpwned website and avoid the API calls entirely:
The entire set of passwords is downloadable for free below with each password being represented as either a SHA-1 or an NTLM hash to protect the original value (some passwords contain personally identifiable information) followed by a count of how many times that password had been seen in the source data breaches. The list may be integrated into other systems and used to verify whether a password has previously appeared in a data breach after which a system may warn the user or even block the password outright.
https://haveibeenpwned.com/Passwords
Given the recent release of the library, I can't say for sure whether users in general will find the mtl interface superfluous or useful. As with many things, I suspect it will be useful for some and not for others. It's been used in multiple codebases by multiple developers and reflects what we think is a fairly common pattern. That gives me some confidence that it's not entirely idiosyncratic.
In the reader case you mention, it could become a bit annoying to have to runPwnedT every time you want to get a password result. You could wrap those lines up in a separate function if you find them unsightly. You could also run this code in a composite monad that satisfies your reader constraint and knows how to check passwords, relieving you of having to call runPwnedT every time (and making it easier for you to switch implementations, should you so desire).
If the dependency on mtl
can be avoided by omitting the transformer, it might be reasonable.
An alternative could be to provide a hibp-transformers
or similar library which provides PwnedT. I think this is a common idiom. Having two libraries might slightly increase maintenance overhead, but reducing dependencies in the main library might reduce it in other ways!
I'm pretty sure any example one can give me that uses the PwnedT
transformer, I can show that it's superfluous and you don't need it. At least in the state this package is in right now.
If you're going to have a type class, use it to make it easier for users to make their own Monads an instance of it, so that it makes it easier to use the main API.
For example:
class MonadPwned m where
getConfig :: m Config
haveIBeenPwned :: MonadPwned m => Text -> m Result
It makes more sense to have the getConfig
in the type class and then the haveIBeenPwned
as a function next to it, so that a user can make an instance to get the Config
from their monad, and then just use the function in their monad.
And if someone would want to make their own implementation of haveIBeenPwned
, why would they even use this library?
The Config
type is specific to the implementation where you connect to a host serving the Have I Been Pwned API. For example, it is not useful for the example given where a snapshot of the database is used. In that case the monad transformer used to provide this functionality would need configuration for the file location or database location of the snapshot, not the current Config
type.
It is possible that a user of this library needs to choose different implementations of the monad transformer under various circumstances. For example, they may wish to write generic user code that will be deployed to environments where external API requests are unacceptable as well as environments where they are acceptable.
I put this in a reply on reddit, but it should (also) be put here.
Here are some points which could make this package better. (IMHO)
REPETITION
Seeing as this module has a very specific name, you can drop all the
/[hH]aveIBeenOwned/
in the names and just advise the user to import this module qualified. That makes the amount the user types be exactly the same: i.e.HaveIBeenPwned.Config
instead ofHaveIBeenOwnedConfig
. PLUS the user can justimport [...] as HIBP
so that they only have to typeHIBP.Config
.TRIMMING
The fields of the
Config
don't need to start with underscores, you're not using lenses, so is this just an oversight?I also don't think the
haveIBeenOwned
function needs to be a method of a type class. It would accomplish the same goal as a function with the following type:(MonadLogger m, MonadIO m) => Config -> Text -> m Result
You can remove the
MonadPwned
type class, remove thePwnedT
data type and the functions that go along with it, don't export thepasswdDigest
andparseHIBPResponse
(because the user doesn't need to ever use those) and you're down to just aConfig
,Result
andhaveIBeenPwned
. Nice and clean, and really easy to understand in one look at the module.ADDITION
Maybe add a
hibpAddress :: Text
with the current URL for convenience? (https://haveibeenpwned.com
)And add a paragraph in the module documentation about this being the URL at the time of creation of that package version, and that if it changes in the future, the user can just provide a different URL in the Config
DOCUMENTATION
It could use a bit more explanation in the module about what the goal is, what HIBP is, a bit of explanation what's used under the hood (for example, that this will throw an exception if the apiHost is not a valid URL) and maybe a really short example code snippet of how to use the module.