Skip to content

extend profiler article about how to use the stopwatch #10332

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

Closed
wants to merge 4 commits into from

Conversation

SimonHeimberg
Copy link
Contributor

No description provided.

@SimonHeimberg
Copy link
Contributor Author

alternative to #9320, was suggested there

@SimonHeimberg SimonHeimberg force-pushed the profilerStopwatch branch 2 times, most recently from fd2f75d to 5f01600 Compare September 12, 2018 08:17
@SimonHeimberg SimonHeimberg changed the base branch from master to 2.8 September 12, 2018 08:20
Copy link
Contributor

@ndench ndench left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the contribution 👍

Copy link
Contributor

@ndench ndench left a comment

Choose a reason for hiding this comment

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

Actually, the build is failing because stopwatch.rst is not being included anywhere:

/home/travis/build/symfony/symfony-docs/profiler/stopwatch.rst:: WARNING: document isn't included in any toctree

Can you include it in profiler.rst the same as the other profiler docs?

@SimonHeimberg
Copy link
Contributor Author

Finally done. The bot is pleased. Sorry for force-pushing this often. I first did not know how to resolve the merge conflict.
Current branch is the original commit merged with 2.8, and then two commits.

@SimonHeimberg
Copy link
Contributor Author

SimonHeimberg commented Nov 29, 2018

What does failure of platformsh mean? Probably nothing about this pr, others fail as well.

@ndench
Copy link
Contributor

ndench commented Nov 30, 2018

Do you want to update profiler.rst to have a link to stopwatch.rst?

@xabbuh
Copy link
Member

xabbuh commented Nov 30, 2018

Thank you for the PR. Do you think we still need it now that #10259 is merged?

@SimonHeimberg
Copy link
Contributor Author

The profiler page would be nice since this is the place I would probably start searching. (Suggested to place the info there in #9320 (review).) But duplicating the information is no help, a link is sufficient.
But dump() is not mentioned on the profiler page. And this is the simplest way to add custom info to the profiler. Should we remain consistent?

summary of history

Well, not sure anymore how history was. I made several PR concerning this topic:
on 2018-02-23 #9320, closed in favour of this one
on 2018-09-05 #10259, merged on 2018-11-30, very similar to #9320, no link to the other two
on 2018-09-12 this one, opened because suggested in #9320 (review)

I guess I created #10259 without remembering the first PR. And having forgotten to mention it when replacing the 1st one. So there were probably two plans...

@xabbuh xabbuh added this to the 3.4 milestone Dec 13, 2018
@javiereguiluz
Copy link
Member

@SimonHeimberg thanks for this contribution! You are right that this is a bit confusing and it's not easy to know where to put things. So, in #10970 we're revamping the main Profiler article. I've been inspired by your work here to create a section about the Stopwatch integration in the profiler. so, we can close this in favor of #10970. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants