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

Add --per-file option #30

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

r-plus
Copy link
Contributor

@r-plus r-plus commented Sep 4, 2018

hi @giginet

I want to add --per-file option to sum time per file.
It is useful to know total compile time for file is reduced when if we refactoring single function to split some small functions.

example

off(default)

$ ./bin/xcprofiler spec/fixtures/MyApp-xxxxxxxxxxx/Logs/Build/valid.xcactivitylog -l 5 -t 20
+--------------------------+------+----------------------+----------+
| File                     | Line | Method name          | Time(ms) |
+--------------------------+------+----------------------+----------+
| Interaction.swift        | 17   | get {}               | 69.9     |
| Matcher.swift            | 7    | get {}               | 68.9     |
| InteractionBuilder.swift | 87   | func clean()         | 49.8     |
| Session.swift            | 74   | public func run(c... | 49.6     |
| Session.swift            | 65   | @discardableResul... | 28.3     |
+--------------------------+------+----------------------+----------+

on

$ ./bin/xcprofiler spec/fixtures/MyApp-xxxxxxxxxxx/Logs/Build/valid.xcactivitylog -l 5 -t 20 --per-file
+--------------------------+------+-------------+----------+
| File                     | Line | Method name | Time(ms) |
+--------------------------+------+-------------+----------+
| Session.swift            | 0    |             | 83.4     |
| InteractionBuilder.swift | 0    |             | 81.5     |
| Interaction.swift        | 0    |             | 73.5     |
| Matcher.swift            | 0    |             | 70.9     |
| ServiceClient.swift      | 0    |             | 66.8     |
+--------------------------+------+-------------+----------+

thanks.

@r-plus
Copy link
Contributor Author

r-plus commented Sep 4, 2018

oops, should I support Ruby 2.3 to pass CI?
(Enumerable#sum method is added since 2.4.)

or drop support 2.3 and add 2.5 to CI matrix to support latest 2 minor versions of Ruby.

@giginet
Copy link
Owner

giginet commented Sep 4, 2018

oops, should I support Ruby 2.3 to pass CI?

Yes. Because default Ruby on macOS is still Ruby 2.3...

I recommend you to use just call reduce instead of sum.

executions.map(:time).reduce(:+)

@r-plus
Copy link
Contributor Author

r-plus commented Sep 4, 2018

okey, I understand support policy for this project.
and thanks for your suggestion to do it, I'll fix it.

@r-plus
Copy link
Contributor Author

r-plus commented Sep 6, 2018

Fixed to support Ruby 2.3.
@giginet Could you review it?

Copy link
Owner

@giginet giginet left a comment

Choose a reason for hiding this comment

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

Execution class represents execution times of each methods. Not them of whole methods in files.
So I think this implementation seems to be bit strange.

Just my idea, you're better to define FileExecutionReporter to show summation.

If --per-file option is passed. using the reporter instead of StandardOutputReporter.

execution.path
}.map { |path, executions|
time = executions.map(&:time).reduce(:+)
Execution.new(time, "#{path}:0:0", '')
Copy link
Owner

Choose a reason for hiding this comment

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

It's bit strange to pass 0 as line number or column...

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.

2 participants