Skip to content
This repository has been archived by the owner on Dec 8, 2023. It is now read-only.

Post contract size diff to parent PR #16

Merged
merged 97 commits into from
Nov 16, 2021
Merged

Conversation

cmichi
Copy link
Contributor

@cmichi cmichi commented Nov 15, 2021

Final step for closing #4 after prior work by @HCastano.

Example of how it looks.

Ideally we would move the number-formatting stuff into csv-comparator, but that can be done in a follow-up if someone feels like it.

@cmichi cmichi marked this pull request as ready for review November 15, 2021 16:25
@cmichi cmichi requested a review from HCastano November 15, 2021 17:59
Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

Small comments, but this is really nice 🦑

.scripts/evaluate-examples-size-changes.sh Outdated Show resolved Hide resolved
.scripts/evaluate-examples-size-changes.sh Outdated Show resolved Hide resolved
csv-comparator $BASELINE_FILE $COMPARISON_FILE | \
sort | \
awk -F"," '{printf "`%s`,%.2f kb,%.2f kb\n", $1, $2, $3}' | \
sed --regexp-extended 's/^([0-9])/,+\1/g' | \
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this doing? My regex brain is too smol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prepends a plus in front of positive numbers. Yeah we could definitely move all of this into csv-comparator. I added a comment as well.

# Append the original optimized size (i.e. not the delta) to the end of each line
cat $COMPARISON_FILE | \
sort | uniq | \
egrep --only-matching ', [0-9]+\.[0-9]+$' | \
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if maybe we should install ripgrep in our CI Docker image, might make some of this stuff more readable

.scripts/evaluate-examples-size-changes.sh Outdated Show resolved Hide resolved
.scripts/evaluate-examples-size-changes.sh Outdated Show resolved Hide resolved
sed --regexp-extended 's/(-+)\:/:\1/' | \
sed 's/$/\\n/g' | \
tr -d '\n' | \
tee contract-size-diff-nl.md
Copy link
Contributor

Choose a reason for hiding this comment

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

What's nl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

newline, I changed the name.

Michael Müller and others added 6 commits November 16, 2021 16:30
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
@cmichi cmichi merged commit 551d57b into master Nov 16, 2021
@cmichi cmichi deleted the cmichi-post-comment-in-pr branch November 16, 2021 15:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants