GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-135953: Add flamegraph reporter to sampling profiler by lkollar · Pull Request #138304 · python/cpython Conversation
|
| .style("position", "absolute") | ||
| .style("padding", "20px") | ||
| .style("background", "white") | ||
| .style("color", "#2e3338") | ||
| .style("border-radius", "8px") | ||
| .style("font-size", "14px") | ||
| .style("border", "1px solid #e9ecef") | ||
| .style("box-shadow", "0 8px 30px rgba(0, 0, 0, 0.15)") | ||
| .style("z-index", "1000") | ||
| .style("pointer-events", "none") | ||
| .style("font-weight", "400") | ||
| .style("line-height", "1.5") | ||
| .style("max-width", "500px") | ||
| .style("font-family", "'Source Sans Pro', sans-serif") | ||
| .style("opacity", 0); |
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.
Is it possible to apply these styles via a class, instead of inline?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| } | ||
|
|
||
| const pythonTooltip = flamegraph.tooltip.defaultFlamegraphTooltip(); | ||
|
|
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| const sourceLines = source | ||
| .map( | ||
| (line) => | ||
| `<div style="font-family: 'SF Mono', 'Monaco', 'Consolas', monospace; font-size: 12px; color: ${line.startsWith("→") ? "#3776ab" : "#5a6c7d"}; white-space: pre; line-height: 1.4; padding: 2px 0;">${line.replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">")}</div>`, |
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.
Likewise, is it possible to use a class instead of inline styles?
Should we escape the HTML (line.replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">")) whilst we're still in Python?
For the color codes, could we use CSS variables so that if we change them in the future, we only need to change it in one place?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| ${ | ||
| calls > 0 | ||
| ? ` | ||
| <span style="color: #5a6c7d; font-weight: 500;">Function Calls:</span> | ||
| <strong style="color: #2e3338;">${calls.toLocaleString()}</strong> | ||
| ` | ||
| : "" | ||
| } |
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.
Perhaps extract to local variables instead of nested substitutions?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| } | ||
|
|
||
| // Wait for libraries to load | ||
| document.addEventListener("DOMContentLoaded", function () { |
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.
My understanding is that DOMContentLoaded may fire before the script loads, so the suggested pattern is to check for document.readyState. I don't know if this applies here or not, though.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| // Add search functionality - wait for chart to be ready | ||
| if (searchInput) { |
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 this block be extracted to a helper function?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| def _create_flamegraph_html(self, data): | ||
| data_json = json.dumps(data) | ||
|
|
||
| template_dir = pathlib.Path(__file__).parent |
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.
Should we use importlib.resources here? I believe e.g. for the Windows embedable builds, the stdlib is distributed as a zip file so modules might not have a __file__ attribute.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Since this is a static template there are no advantages of doing this and having the web parts in Python is a bit messy specially since we cannot use something like Jinja. Having all the web parts in JS or the template itself is a common practice for this case and keeps the Python code focused just on the data part and not in creating HTML. Also since this is just a static file served with no dependencies so there are no concerns aboutsecurity or injection so I prefer to maximize maintainability. I will give it a pass with prettier to format it as you indicate :) |
All reactions
-
👍 1 reaction
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Thanks for the review @AA-Turner. I think I addressed everything except for the comments around constructing HTML in JS. I also ended up breaking up the JS functions a bit in 7c4d792 as it was getting really unwieldy and I think this will make maintenance easier. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
ff7c603 to
9d3956d
Compare
|
Apologies, I messed up the rebase. I'll open a new PR to keep a clean slate. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
4 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.