-
Notifications
You must be signed in to change notification settings - Fork 114
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
style settings configuration #52
style settings configuration #52
Conversation
Just a note that you should probably do your work on top of @JordiChauzi's changes in #32, since they affect a lot of the style code! |
Sounds like a good plan! I just had a skim, and the move to I think we'll also want to try to be a bit more helpful with descriptions of the flags than |
@AnderEnder #32 has now landed! |
b72f56f
to
260c368
Compare
Also, just as a side note, now that we have comments pending, it'd be great if you can avoid force pushing. That way I can review just what has changed, rather than having to review the whole thing all over again :) We'll squash before merging anyway so the history remains clean! |
Oh, now that we've had some other PRs land, there are also some conflicts with |
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
=========================================
+ Coverage 89.66% 90.57% +0.9%
=========================================
Files 16 16
Lines 1558 1708 +150
=========================================
+ Hits 1397 1547 +150
Misses 161 161
Continue to review full report at Codecov.
|
@AnderEnder this is looking really good now! Only the one typo ("Chart" vs "Graph") and the JavaScript escaping left I think :) |
and test cases :) |
@AnderEnder I reorganized and removed the duplication in the flamegraph tests in 59a9dac. It might be easier to revert your changes to the testing code, then merge |
@jonhoo I agree that we shouldn't need to include the JavaScript in all of the tests since it's static. Do you think we should just add a |
Yeah, something like that. Probably make it |
@jonhoo and @jasonrhansen |
@AnderEnder I agree that finding differences in the XML isn't nice since it's all on one line. For |
38e520e
to
c949900
Compare
|
We can use If we do it for tests only, should we add another flag for pretty-printed XML, or just add one flag for testing that both prettifies the XML and doesn't include the JavaScript? The way I see it, a user would never want to generate a flame graph without JavaScript, but they might want to generate one with pretty XML. We could have the What are your thoughts? |
Also, we probably want to still include the JavaScript output of this in the tests since the values are dynamic. svg.write_event(Event::CData(BytesText::from_escaped_str(&format!(
"\
var nametype = '{}';
var fontsize = {};
var fontwidth = {};
var xpad = {};
var inverted = {};
var searchcolor = 'rgb(230,0,230)';",
opt.name_type.replace("'", "\\'"),
opt.font_size,
opt.font_width,
super::XPAD,
opt.direction == Direction::Inverted
))))?; |
And yes, we want that segment of JavaScript to be included, just not the giant JS file that's loaded with |
So in the library we can use #[structopt(raw(hidden = "true"), long = "no-javascript")]
no_javascript: bool |
I don't think we need the flag to be present at all in the CLI. All the tests operate directly on the library, no? |
We don't necessarily need the flag on the CLI, but it would make it easier to create the test SVG files that our tests compare to. I suppose removing the JavaScript manually from the SVGs is easy enough though. |
Hmm, I suppose that's a good point. I think I'd be okay with a hidden CLI flag too then. Maybe with a help text that explains exactly what the intended use-case is. |
@AnderEnder I'm working on reorganizing the test data files. While I'm at it I can add the |
I've made those changes in #70. |
@AnderEnder a gentle ping on this -- it's a pretty wide change, and it will probably only get more painful to try to merge as time goes on. I think the only real remaining comment now is whether we simply import the Also, I think @jasonrhansen's changes in #70 may let you simplify the tests somewhat, but he can advise on that better than I can. |
@AnderEnder #70 changes the flamegraph tests to not include JavaScript and to pretty-print the XML so it's easy to see diffs when there is a test failure. Because of this, you'll need to regenerate the test SVGs using the following flags The tests directory structure has also been altered. The files in |
I just did the merge with |
This PR adds most of the styling options listed in jonhoo#16.
060d692
to
cfefd3c
Compare
@jasonrhansen I believe this is now finally ready for review 🎉 |
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.
It all looks good to me, except for my comment about putting DEFAULT_TITLE
in quotes for structopt
. But that could still be done in a separate PR.
👍 Filed as #91. |
🎉 |
Thanks for all your work on this @AnderEnder ! |
Work in progress: implementing some of arguments setting from #16