-
Notifications
You must be signed in to change notification settings - Fork 48
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
fix(docs): Write user-centric documentation #54
Conversation
@@ -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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
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 |
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 |
To try to get some of these changes approved, would you be able to separate it into smaller PRs? Thanks again for the time. |
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. |
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. |
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. 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: |
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! |
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 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 |
After looking more closely, I feel each function's doc is far too similar. Crate level documentation would alleviate this. |
This PR
Fixes #53
Details
Iter
- see fix: Publishcrate::iter::Iter
ascrate::Iter
. #51).Result
returns explicit where they were previously inconsistent with other parts of the crate.