-
Notifications
You must be signed in to change notification settings - Fork 151
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
Fix/go tabled benchmarks #32
Fix/go tabled benchmarks #32
Conversation
See https://regex101.com/r/kOwwOf/1 for a quick look at the effects of the regex change. |
Any chance this could be merged soon? :) |
Evidently, no. |
@rhysd could you point out any potential issues with this PR? |
For now, I've worked around this by hackily renaming the benchmark names, replacing - name: Run benchmarks
run: make bench | tee output.raw
- name: Fix benchmark names
run: >-
perl -pe 's/^(Benchmark.+?)\/(\S+)(-\d+)(\s+)/\1__\2\4/' output.raw |
tr '=-' '_' | tee output.txt
- name: Store benchmark result
uses: rhysd/github-action-benchmark@v1
with:
tool: "go"
output-file-path: output.txt
github-token: ${{ secrets.GH_PUSH_TOKEN }}
comment-on-alert: true
auto-push: true Full source is here. |
That's neat and sad at the same time. |
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.
Thank you for your contribution 👍🏼 I've added a couple of suggestions. Let me know what do you think
pinging @kaancfidan this time :) |
Co-authored-by: Chris Trześniewski <k.trzesniewski@gmail.com>
Co-authored-by: Chris Trześniewski <k.trzesniewski@gmail.com>
I had stopped hoping this PR would get merged and somehow missed it got attention. |
Thanks, @kaancfidan for applying the suggestions. I've recently taken over the maintenance of this project and I'm trying to wrap up all the outstanding PRs |
@ktrz although I have already applied the change; I have checked your suggested regex in regex101.com and it seems to cause problems there: https://regex101.com/r/kOwwOf/2 Could this be a sign that the suggested regex is too complex? |
@kaancfidan we didn't account for |
yep, it solves it: https://regex101.com/r/kOwwOf/3 |
Co-authored-by: Chris Trześniewski <k.trzesniewski@gmail.com>
Extended regex to also include tabled benchmarks.
Fixes #31