allan2 / dotenvy

A well-maintained fork of the Rust dotenv crate
MIT License
625 stars 39 forks source link

fix(docs): Write user-centric documentation #54

Closed LeoniePhiline closed 1 year ago

LeoniePhiline commented 1 year ago

This PR

Fixes #53

Details

github-actions[bot] commented 1 year ago

Code Coverage

allan2 commented 1 year ago

Thank you for the contribution.

Please keep in mind that this is a small crate that hopefully is straightforward to use. I hope the documentation reflects this by not trying to explain too much.

LeoniePhiline commented 1 year ago

The documentation failed to explain essential behavior. This is fixed now.

The documentation does not go into any more detail than what library users need to know to be able to use the provided functionality.

I would recommend you run cargo doc --open and actually have a look at the generated documentation. I promise you, it is much, much better now.

allan2 commented 1 year ago

I did take a look. I feel that it is too verbose.

In some functions, I can see it being useful to explain the error using a sentence or two (such as in dotenv() or var(), but it's not needed everywhere.

allan2 commented 1 year ago

To try to get some of these changes approved, would you be able to separate it into smaller PRs? Thanks again for the time.

LeoniePhiline commented 1 year ago

Which worries do you have exactly?

As you are being quite vague, I wouldn't know what to change. To me, it would be a loss for the user if we removed some of the clarity.

I could also imagine having @sonro adding another viewpoint.

I find it relevant to consider that the documentation is not read from top to bottom like a book.

Instead, as a user you are presented with a selection of functionality and your goal is to triage quickly which functions are relevant to you and which are not.

Each function from the menu of this crate's offerings must document its use case, its distinction from its alternatives, its behavior, including reasons for errors (and potential panics, but we avoid these) and pitfalls (such as files being read once and later requests being answered from cache).

Helpful hints about use cases and alternatives aid the user in finding the right API method for their personal requirements.

With the added headings, the documentation is well structured. It is easy to find the section containing the information one is looking for.

The structure is also consistent across the crate documentation, helping users to easily compare and understand the differences between the functions.

All of this makes for high quality documentation. You find the same in other essential crates as well as notably the standard library.

allan2 commented 1 year ago

The main thing is that it makes the docs too long. I mentioned above that it is excessive to include an Error section for this small crate.

I'm rejecting this PR. Feel free to open separate issues if there are specific places where you find the docs to be lacking.

LeoniePhiline commented 1 year ago

@allan2

See for example #53

Why would you reject an entire PR without requesting specific changes?

The lost value is immense, due to the huge amount of users of this crate! (1,066,771 downloads at this point.) This is a highly demanded fork of an essential crate, offering very basic functionality needed by very many developers.

The current documentation is very lacking - see the "Details" section at the top of this PR. Now consider the many thousand users (including many new rustaceans) who could benefit from this improved documentation.

How do you propose to tackle https://rust-lang.github.io/api-guidelines/documentation.html#function-docs-include-error-panic-and-safety-considerations-c-failure instead of how it is done in this PR?

Which effect do "too long" docs have? How can the issue of "too long docs" be resolved, so that everyone (> 1M downloads of all time!) can still gain the benefit of vastly improved documentation? Can you please be specific?

The documentation will also reduce your burden as maintainer: Many questions people would ask otherwise would now be answered by the documentation -- ready to be read by anyone at any time, without requiring your time and energy.

allan2 commented 1 year ago

Perhaps reject was too strong a word. I closed this PR because it would be difficult to accept because of its scope.

It would be easier to get changes merged if they were split into smaller, focused ones. For example, you could open one for the formatting fixes, or another for #53. I would suggest opening issues first instead of a PR to help facilitate discussion before time is spent on the implementation.

When the docs are too long, it can overload the reader. It can also make it harder to find the useful things.

@LeoniePhiline thanks again for the effort. It really is appreciated!

sonro commented 1 year ago

I am taking a look at the changes proposed here. From a quick glance they do seem overly verbose related to the size of the crate. However, some of these changes are great.

I feel a lot of the duplicataion could be avoided with better crate-level information and examples. We can link to those from within the functions docs. This idea, along with others, could be workshopped better in issues before someone dedicates time into implementing their idea.

I would suggest opening issues first instead of a PR to help facilitate discussion before time is spent on the implementation.

I think it is time we create a CONTRIBUTING file, and assign people to issues to avoid duplicate work - as I too have been writing updated documentation. See #56

sonro commented 1 year ago

After looking more closely, I feel each function's doc is far too similar. Crate level documentation would alleviate this.