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

Added options to make PR compare against base branch and then output result #17

Merged
merged 4 commits into from
Mar 13, 2020
Merged

Conversation

pksunkara
Copy link
Contributor

@pksunkara pksunkara commented Mar 5, 2020

clap-rs/clap#1724 is the PR you can see.

As you can see there, both the options are working.

@pksunkara pksunkara changed the title Added comment-always option Added options to make PR compare against base branch and then output result Mar 5, 2020
@rhysd
Copy link
Member

rhysd commented Mar 5, 2020

Thank you for adding this. Let me review this PR this weekend.

@rhysd rhysd self-assigned this Mar 5, 2020
@pksunkara
Copy link
Contributor Author

Hey, any update on this?

@rhysd
Copy link
Member

rhysd commented Mar 10, 2020

I'm sorry for the delay. I'm now focusing on another private project and could not review this last weekend. Let me check now.

README.md Show resolved Hide resolved
src/extract.ts Show resolved Hide resolved
src/write.ts Outdated Show resolved Hide resolved
src/write.ts Outdated Show resolved Hide resolved
@pksunkara
Copy link
Contributor Author

I removed the saveOnPr option and used autoPush. But it would break backward compatibility for people who don't have autoPush set to true and relying that their benchmarks are stored to external data files.

src/write.ts Show resolved Hide resolved
test/write.ts Outdated Show resolved Hide resolved
@rhysd
Copy link
Member

rhysd commented Mar 11, 2020

Thanks for the responses. I also left a comment in outdated conversation. #17 (comment)

@pksunkara
Copy link
Contributor Author

@rhysd I have addressed your review. The only thing left to decide is #17 (comment). I really want some customization here because it looks really ugly otherwise.

@codecov-io
Copy link

Codecov Report

Merging #17 into master will decrease coverage by 3.63%.
The diff coverage is 45.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
- Coverage   92.27%   88.64%   -3.64%     
==========================================
  Files           5        5              
  Lines         518      546      +28     
  Branches       95      101       +6     
==========================================
+ Hits          478      484       +6     
- Misses         18       37      +19     
- Partials       22       25       +3
Impacted Files Coverage Δ
src/extract.ts 88.82% <100%> (ø) ⬆️
src/write.ts 84.57% <40.54%> (-8.58%) ⬇️
src/config.ts 93.49% <50%> (-1.47%) ⬇️

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 6112559...dce7a31. Read the comment docs.

Copy link
Member

@rhysd rhysd 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. Thank you for the updates.

@rhysd rhysd merged commit f7e7460 into benchmark-action:master Mar 13, 2020
@rhysd
Copy link
Member

rhysd commented Mar 13, 2020

@pksunkara I merged this. Thank you for your contribution. I'll make a new release tomorrow.

@rhysd
Copy link
Member

rhysd commented Mar 17, 2020

I'm sorry for the delay. I released new version v1.8.0 including this patch.

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.

None yet

3 participants