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

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

Conversation

LeoniePhiline
Copy link

@LeoniePhiline LeoniePhiline commented Dec 11, 2022

This PR

Fixes #53

Details

@github-actions
Copy link

Code Coverage

@@ -15,6 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

- Minimum Supported Rust Version is now 1.56.1
- Removed internal `dotenv_codegen_impl` crate and `proc_macro_hack` dependency
- Rewrote most documentation ([PR #54](https://github.com/allan2/dotenvy/pull/54) by [LeoniePhiline](https://github.com/LeoniePhiline)
Copy link
Owner

Choose a reason for hiding this comment

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

Could you be more specific as to how the documentation was changed?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure what you mean. What would you propose? Something like the list at the top of this PR? This seems a little verbose to me. This is a changelog - users commonly don't use it to find out which documentation was changed in detail, but rather which type interface and behavior changes affect them.

@allan2
Copy link
Owner

allan2 commented Dec 12, 2022

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
Copy link
Author

LeoniePhiline commented Dec 13, 2022

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
Copy link
Owner

allan2 commented Dec 13, 2022

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
Copy link
Owner

allan2 commented Dec 13, 2022

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
Copy link
Author

LeoniePhiline commented Dec 13, 2022

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
Copy link
Owner

allan2 commented Dec 13, 2022

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.

@allan2 allan2 closed this Dec 13, 2022
@LeoniePhiline
Copy link
Author

@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
Copy link
Owner

allan2 commented Dec 13, 2022

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
Copy link
Collaborator

sonro commented Dec 14, 2022

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
Copy link
Collaborator

sonro commented Dec 15, 2022

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

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.

Documentation does not explain difference between from_filename and from_path
3 participants