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

Implement --nameattr option #39

Merged
merged 13 commits into from
Feb 10, 2019
Merged

Implement --nameattr option #39

merged 13 commits into from
Feb 10, 2019

Conversation

jasonrhansen
Copy link
Collaborator

Implements #24

@codecov-io
Copy link

codecov-io commented Feb 8, 2019

Codecov Report

Merging #39 into master will increase coverage by 40.53%.
The diff coverage is 89.94%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #39       +/-   ##
===========================================
+ Coverage   41.81%   82.35%   +40.53%     
===========================================
  Files           7        9        +2     
  Lines         672      867      +195     
===========================================
+ Hits          281      714      +433     
+ Misses        391      153      -238
Impacted Files Coverage Δ
tests/collapse-perf.rs 93.61% <ø> (ø)
src/bin/flamegraph.rs 0% <0%> (ø) ⬆️
src/flamegraph/mod.rs 84.07% <100%> (+84.07%) ⬆️
tests/flamegraph-nameattr.rs 86.95% <86.95%> (ø)
src/flamegraph/attrs.rs 89.74% <89.74%> (ø)
src/flamegraph/merge.rs 84.84% <0%> (+84.84%) ⬆️
src/flamegraph/svg.rs 98.36% <0%> (+98.36%) ⬆️

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 ce1c41a...96ba2c1. Read the comment docs.

src/flamegraph/attrs.rs Outdated Show resolved Hide resolved
src/flamegraph/attrs.rs Outdated Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Feb 8, 2019

This looks pretty good! I think I'd like to see a test that actually passes in a nameattr file and checks the SVG output that those attributes are actually there, but that's my only major piece of feedback :)

src/flamegraph/attrs.rs Outdated Show resolved Hide resolved
src/flamegraph/attrs.rs Outdated Show resolved Hide resolved
src/flamegraph/attrs.rs Outdated Show resolved Hide resolved
src/flamegraph/attrs.rs Outdated Show resolved Hide resolved
src/flamegraph/attrs.rs Outdated Show resolved Hide resolved
@jasonrhansen
Copy link
Collaborator Author

I would still like to add a test that checks the SVG output since we're currently only testing the parsing.

src/flamegraph/attrs.rs Outdated Show resolved Hide resolved
@jonhoo jonhoo merged commit 744d116 into jonhoo:master Feb 10, 2019
@jonhoo
Copy link
Owner

jonhoo commented Feb 10, 2019

No idea why CI was sad. I've merged now and then I guess we should watch https://travis-ci.org/jonhoo/inferno/builds/491312148.

@jonhoo
Copy link
Owner

jonhoo commented Feb 10, 2019

Oh, and thanks! :D

@jasonrhansen jasonrhansen deleted the nameattr branch February 12, 2019 18:29
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.

3 participants