Skip to content

Conversation

@jonhoo
Copy link
Owner

@jonhoo jonhoo commented Feb 25, 2019

I've made a pass on pretty much all the docs, as well as the names of a few types, to try to clean up the externally-visible API. It'd be awesome if someone could give it all a read through and see that a) the structure of the API makes sense, b) I'm not saying anything totally false, and c) there aren't any gaping holes that we should fill immediately.

I have included any example code somewhat intentionally since I'm guessing the external APIs are likely to change a decent amount with #30.

@jonhoo jonhoo added help wanted Extra attention is needed docs Updating or improving documentation labels Feb 25, 2019
@codecov-io
Copy link

codecov-io commented Feb 25, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@65587d8). Click here to learn what that means.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #61   +/-   ##
=========================================
  Coverage          ?   75.82%           
=========================================
  Files             ?       14           
  Lines             ?     1212           
  Branches          ?        0           
=========================================
  Hits              ?      919           
  Misses            ?      293           
  Partials          ?        0
Impacted Files Coverage Δ
src/collapse/mod.rs 0% <ø> (ø)
src/flamegraph/color/palette_map.rs 0% <ø> (ø)
src/bin/collapse-perf.rs 0% <0%> (ø)
src/flamegraph/mod.rs 82.6% <100%> (ø)
src/flamegraph/attrs.rs 89.74% <100%> (ø)
tests/collapse-perf.rs 93.87% <100%> (ø)
src/collapse/perf.rs 81.5% <100%> (ø)
src/flamegraph/color/mod.rs 71.65% <80%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65587d8...924421b. Read the comment docs.

Copy link
Collaborator

@jasonrhansen jasonrhansen left a comment

Choose a reason for hiding this comment

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

Looks really good to me! Other than the few typos I pointed out I have nothing to add.

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 25, 2019

@jasonrhansen great, thanks for the review! I'll leave this open for a bit longer in case @JordiChauzi also has a chance to take a look, otherwise I'll merge towards the end of today EST.

@JordiChauzi
Copy link
Collaborator

For different structures implementing Default, you precised the default value in the doc of the structure. Hopefully the value will not change overtime, but is it not redundant ?

Copy link
Collaborator

@JordiChauzi JordiChauzi left a comment

Choose a reason for hiding this comment

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

The documentation is really clear and complete (minus the possible examples that we may add later).
Seems good to me.

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 25, 2019

I think it's useful to spell out the defaults in the docs since they're not otherwise visible anywhere beyond digging into the code. It's true that we'd have to make sure update the docs if the defaults change, but at the same time I think these defaults should also be relatively unlikely to change :)

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 25, 2019

Thanks for the review from both of you! I'll merge once CI passes!

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 25, 2019

@jasonrhansen I added some more docs following #60 in 3e706ea. Can you take a quick look?

@jasonrhansen
Copy link
Collaborator

Thanks for adding docs for differentials! I made "Flame Graph" the default for the title in Options. We should probably document that now.

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 25, 2019

@jasonrhansen done in 924421b

@jonhoo jonhoo merged commit 4a1fd63 into master Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Updating or improving documentation help wanted Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants