search

chanced / jsonptr

JSON Pointer (RFC 6901) implementation for Rust
Apache License 2.0
44 stars 4 forks source link

Improve docs & codecov for 0.5.0 #56

Closed chanced closed 3 months ago

chanced commented 4 months ago
codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 99.89529% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.1%. Comparing base (a67948d) to head (b90b5e3).

Additional details and impacted files | [Files](https://app.codecov.io/gh/chanced/jsonptr/pull/56?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance) | Coverage Δ | | |---|---|---| | [src/assign.rs](https://app.codecov.io/gh/chanced/jsonptr/pull/56?src=pr&el=tree&filepath=src%2Fassign.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance#diff-c3JjL2Fzc2lnbi5ycw==) | `99.6% <100.0%> (+3.4%)` | :arrow_up: | | [src/component.rs](https://app.codecov.io/gh/chanced/jsonptr/pull/56?src=pr&el=tree&filepath=src%2Fcomponent.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance#diff-c3JjL2NvbXBvbmVudC5ycw==) | `100.0% <100.0%> (ø)` | | | [src/delete.rs](https://app.codecov.io/gh/chanced/jsonptr/pull/56?src=pr&el=tree&filepath=src%2Fdelete.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance#diff-c3JjL2RlbGV0ZS5ycw==) | `99.4% <100.0%> (+1.7%)` | :arrow_up: | | [src/index.rs](https://app.codecov.io/gh/chanced/jsonptr/pull/56?src=pr&el=tree&filepath=src%2Findex.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance#diff-c3JjL2luZGV4LnJz) | `100.0% <100.0%> (+61.9%)` | :arrow_up: | | [src/resolve.rs](https://app.codecov.io/gh/chanced/jsonptr/pull/56?src=pr&el=tree&filepath=src%2Fresolve.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance#diff-c3JjL3Jlc29sdmUucnM=) | `100.0% <100.0%> (+10.8%)` | :arrow_up: | | [src/token.rs](https://app.codecov.io/gh/chanced/jsonptr/pull/56?src=pr&el=tree&filepath=src%2Ftoken.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance#diff-c3JjL3Rva2VuLnJz) | `100.0% <100.0%> (+22.2%)` | :arrow_up: | | [src/pointer.rs](https://app.codecov.io/gh/chanced/jsonptr/pull/56?src=pr&el=tree&filepath=src%2Fpointer.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance#diff-c3JjL3BvaW50ZXIucnM=) | `98.1% <99.7%> (+20.6%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/chanced/jsonptr/pull/56/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chance)
chanced commented 4 months ago

@asmello I think flattening the crate, except for resolve, assign, delete, where the traits are re-exported at the root, makes sense. This means eliminating the index mod though, so we would need to move the excellent documentation you've already written.

The reasoning for this is that I've found folks tend to like a well structured hierarchy when working on project but prefer a flatter surface API when utilizing a library. I'm still rather novice to rust, so it could be that Rustaceans disagree with this. If you feel like that's the case, I think having a token mod that houses Token, Tokens, Index, and any respective errors. I don't know where we'd put Component and Components though.

I had gotten the code coverage to 99% before introducing Component / Components. Both are untested (and undocumented) so we are sitting at 98.2%. I'll get it back up over 99% before this release is cut. I don't know if I can hit 100% tho.

I've written some crate-level docs. They are currently being imported from the README.md but I plan on changing that. I'm just using markdown for ease of writing, formatting, and highlighting. Once the verbiage is settled, I'll move them to doc comments and make the README.md more generic.

I'm headed out to the river but I should hopefully be able to get this finished over the weekend or early next week.

asmello commented 4 months ago

@asmello I think flattening the crate, except for resolve, assign, delete, where the traits are re-exported at the root, makes sense. This means eliminating the index though, so we would need to move the excellent documentation you've already written.

Not sure I follow what you're saying. I'm ok with having the commonly used symbols exported at the root; that is common practice in library crates. If not best practice. This doesn't preclude having a hierarchical project structure though, and that is also best practice. Even if a module is not public it makes sense for it to be relatively well documented.

One rule I'd hold though is to only publicly export each symbol at one path. This can be the root, but if it is then module the symbol comes from shouldn't also be public.

I had gotten the code coverage to 99% before introducing Component / Components. Both are untested (and undocumented) so we are sitting at 98.2%. I'll get it back up over 99% before this release is cut. I don't know if I can hit 100% tho.

I think not hitting 100% is ok. Lack of coverage indicates code isn't tested, which isn't good, but having coverage doesn't prove tests are useful, only that they exist. So while having a relatively high coverage is a worthy objective, it gives decreasing returns past a certain point.

Once the verbiage is settled, I'll move them to doc comments and make the README.md more generic.

I actually like this pattern of having doc-tests in the README. It encourages keeping the README up-to-date as code changes, and lots of crates do this. Is there a particular reason you want to avoid it?

BTW minor point of feedback, but I'd avoid chore-style PRs like this from also including new code like Components. I can handle the diff, but it makes tracking down bugs harder down the line. I also find small PRs useful for contributors to learn from.

chanced commented 4 months ago

Not sure I follow what you're saying. I'm ok with having the commonly used symbols exported at the root; that is common practice in library crates. If not best practice. This doesn't preclude having a hierarchical project structure though, and that is also best practice. Even if a module is not public it makes sense for it to be relatively well documented.

Oh, I didn't mean to flatten it all into lib.rs. I considered it initially, because I've read some people prefer that, but I have an increasingly hard time navigating massive documents. I just meant publicly exposed. So the only 3 mods would be assign, resolve, and delete. I understand that my understanding of hierarchy preferences may not match up in rust.

BTW minor point of feedback, but I'd avoid chore-style PRs like this from also including new code like Components. I can handle the diff, but it makes tracking down bugs harder down the line. I also find small PRs useful for contributors to learn from.

Yea, I regret this. The only reason I did was because I wanted to be able to reference Component/Components in the docs and they didn't exist. I'll delete them from this PR and introduce them.

I actually like this pattern of having doc-tests in the README. It encourages keeping the README up-to-date as code changes, and lots of crates do this. Is there a particular reason you want to avoid it?

The reason I was considering changing this was that the README.md could be made more generic, using terms such as "for Rust," without seeming out of place. All of my projects up to this point have used README and I genuinely love the practice - especially that it keeps the README in check.

Scratch the plan to move the docs out of README.md. We will keep that practice.

asmello commented 4 months ago

Oh, I didn't mean to flatten it all into lib.rs. I considered it initially, because I've read some people prefer that, but I have an increasingly hard time navigating massive documents. I just meant publicly exposed. So the only 3 mods would be assign, resolve, and delete. I understand that my understanding of hierarchy preferences may not match up in rust.

If you mean only those two would be exported as wholesale mods, that's probably ok. I don't think there are enough types that we need a hierarchy in the public API.

Yea, I regret this. The only reason I did was because I wanted to be able to reference Component/Components in the docs and they didn't exist. I'll delete them from this PR and introduce them.

No need to revert now, just for future reference.

The reason I was considering changing this was that the README.md could be made more generic, using terms such as "for Rust," without seeming out of place. All of my projects up to this point have used README and I genuinely love the practice - especially that it keeps the README in check.

Scratch the plan to move the docs out of README.md. We will keep that practice.

Yeah it's a bit of a mixed bag, I've also seen projects use a fancy README that doesn't really work well in docs.rs... I guess we don't need to use the same README for docs.rs and for Github, let me think a bit.

asmello commented 4 months ago

I think we can take a clue from here: https://linebender.org/blog/doc-include/

They have a single README.md that renders differently in docs.rs and in Github. The main trick seems to be using some inline html tags with <div class="rustdoc-hidden">. Seems a bit more advanced so maybe leave it for later.

chanced commented 4 months ago

Oh that's cool! I agree though, let's just have the README pull double duty.

chanced commented 4 months ago

Lack of coverage indicates code isn't tested, which isn't good, but having coverage doesn't prove tests are useful, only that they exist. So while having a relatively high coverage is a worthy objective, it gives decreasing returns past a certain point.

Absolutely. I don't believe that having a really high level of code coverage offers the sort of guarantees on correctness that it may seem. I do tend to shake out small issues when I strive to hit above > 90% though.

chanced commented 3 months ago

@asmello this is ready for you to review whenever you have time.

chanced commented 3 months ago

Hmm, looking at the README, I'm kinda second guessing not using the strategy you linked.

chanced commented 3 months ago

I went ahead and employed the rustdoc hack.

chanced commented 3 months ago

I feel like a few tests are too trivial, which is what I feared was going to happen when trying to reach near 100% coverage.

Yea, my silly ass pursuit of 100 definitely resulted in some obnoxious tests. There were a few tests which I genuinely did not want to write. There were a few, mostly around error outputs, that I let copilot spit out. Yet, I pushed ahead thinking I might actually be able to hit 100 on this lib.

As annoying and useless as some of the tests are, one of the tests that I absolutely thought was pointless caught an issue with TryFrom.

I'm okay with dumping the useless ones. You've marked a few of them.

chanced commented 3 months ago

I created two issues for the last outstanding conversations: https://github.com/chanced/jsonptr/issues/57 and https://github.com/chanced/jsonptr/issues/58