Skip to content

Conversation

@felipesanches
Copy link
Contributor

No description provided.

@lgtm-com
Copy link

lgtm-com bot commented May 11, 2021

This pull request introduces 2 alerts when merging ab89b80 into 5bf40dd - view on LGTM.com

new alerts:

  • 1 for Module imports itself
  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented May 11, 2021

This pull request introduces 1 alert when merging e686528 into 5bf40dd - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@felipesanches
Copy link
Contributor Author

@chrissimpkins This is good for being reviewed now, I think

@chrissimpkins
Copy link
Member

Thanks Felipe! Sorry, I didn't have PR CI support configured. Do you mind rebasing on 8c013cf and force pushing again to trigger the CI?

@felipesanches
Copy link
Contributor Author

ok, I'll do that now

@felipesanches felipesanches force-pushed the dehint_function branch 2 times, most recently from 68ef6d7 to ee8d9a7 Compare May 11, 2021 23:58
@felipesanches
Copy link
Contributor Author

why does it say "5 workflows awaiting approval"? Do you need to add me as an approved contributor in this repo? I've never seen this kind of "pending" status on PRs.

@chrissimpkins
Copy link
Member

I think it must be see something that GitHub put in place after the cryptomining shenanigans

Copy link
Member

@chrissimpkins chrissimpkins left a comment

Choose a reason for hiding this comment

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

Looks great Felipe. Couple of requests and then we can merge this.

@chrissimpkins
Copy link
Member

Is it helpful to expose the pre- / post-execution file size results? IIRC this was being tested by fontbakery. Do you have that covered FB side?

@chrissimpkins
Copy link
Member

chrissimpkins commented May 12, 2021

Is it helpful to expose the pre- / post-execution file size results? IIRC this was being tested by fontbakery. Do you have that covered FB side?

Nvm. I see it in https://github.com/googlefonts/fontbakery/pull/3271/files#diff-bf950becbb6a50067f8e4b91219be2e719ab1ac47e3dbd429a24e63f54b8e9f0R309

@chrissimpkins
Copy link
Member

It's ok to push new commits rather than squashing. It makes it easier to review the changes that you make to address the review.

@chrissimpkins
Copy link
Member

Thank you very much Felipe! I'll merge this and get a new release out tonight

@chrissimpkins chrissimpkins merged commit 5809c88 into source-foundry:master May 12, 2021
@felipesanches
Copy link
Contributor Author

thanks! :-D

@chrissimpkins
Copy link
Member

Adding a new --quiet flag to the executable based on your changes.

#64

Thanks for adding this!

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants