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

refactor(metrics): rename option property from raiseOnEmptyMetrics to throwOnEmptyMetrics #416

Merged
merged 4 commits into from
Jan 7, 2022

Conversation

kozub
Copy link
Contributor

@kozub kozub commented Jan 5, 2022

Description of your changes

How to verify this change

Make sure that all existance of "raiseOnEmptyMetrics" were renamed.

Related issues, RFCs

#348

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published in downstream module
  • The PR title follows the conventional commit semantics

Breaking change checklist

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@dreamorosi dreamorosi requested a review from flochaz January 5, 2022 19:01
@dreamorosi dreamorosi added the metrics This item relates to the Metrics Utility label Jan 5, 2022
@dreamorosi
Copy link
Contributor

Thanks a lot for your first contribution @kozub!

Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Left some minor comments & suggestions but looking forward to approve this!

packages/metrics/src/Metrics.ts Show resolved Hide resolved
packages/metrics/src/Metrics.ts Show resolved Hide resolved
packages/metrics/examples/empty-metrics.ts Outdated Show resolved Hide resolved
packages/metrics/examples/decorator/empty-metrics.ts Outdated Show resolved Hide resolved
docs/core/metrics.md Show resolved Hide resolved
@dreamorosi dreamorosi requested review from ijemmy and removed request for flochaz January 6, 2022 10:45
@kozub
Copy link
Contributor Author

kozub commented Jan 6, 2022

Hey @dreamorosi! Thanks for your suggestions. I've added second commit with your comments applied. Please let me know if you prefer to have all that changes as single commit - I can squash it before mering. I'm not sure what's your preference for this repo.

dreamorosi
dreamorosi previously approved these changes Jan 6, 2022
Copy link
Contributor

@dreamorosi dreamorosi 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 to me, thanks for applying the changes. Once we have a second review from another maintainer we'll squash on merge!

Copy link
Contributor

@ijemmy ijemmy left a comment

Choose a reason for hiding this comment

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

Thanks a lot! This issue has been bugging me but we were too busy with the beta release.

I find 3 more lines. Then we are good to go. (Sorry that examples don't follow the same pattern, we are working on this).

grep -rn -i "raise" . --exclude=\*venv\* --exclude=\*.git\* --exclude=\*node_modules\* --exclude=\*cdk.out\* --exclude=\*.js\* --exclude=\*.d.ts\* --exclude=\*.html\*

./examples/cdk/lib/example-function.MyFunction.ts:32:  metrics.raiseOnEmptyMetrics();
./examples/cdk/lib/example-function.MyFunction.ts:41:  metrics.raiseOnEmptyMetrics();
./examples/cdk/lib/example-function.MyFunctionWithDecorator.ts:18:    raiseOnEmptyMetrics: true,

@kozub
Copy link
Contributor Author

kozub commented Jan 6, 2022

Hey @ijemmy - I've changed method signatures that you mentioned about. Thanks for finding it! I'd tried to search in the project for 'raise' before using VSCode with no results. Looks that I should not trust it ;-)

@dreamorosi
Copy link
Contributor

Hey @ijemmy - I've changes method signatures that you mentioned about. Thanks for finding it! I'd tried to search in the project for 'raise' before using VSCode with no results. Looks that I should not trust it ;-)

Yea, I did the same and also didn't find them, nice one @ijemmy

@dreamorosi dreamorosi self-requested a review January 6, 2022 17:26
dreamorosi
dreamorosi previously approved these changes Jan 6, 2022
ijemmy
ijemmy previously approved these changes Jan 7, 2022
Copy link
Contributor

@ijemmy ijemmy left a comment

Choose a reason for hiding this comment

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

@kozub No problem. It's the same for me. VSCode seems to miss these.

I had this issue before and never trusted its search function since then.

@ijemmy ijemmy dismissed stale reviews from dreamorosi and themself via d0968a5 January 7, 2022 06:51
Copy link
Contributor

@ijemmy ijemmy left a comment

Choose a reason for hiding this comment

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

There was a merge conflict. I have merged from main, resolved, and approved.

@dreamorosi Can I have a new approval?

@dreamorosi dreamorosi self-requested a review January 7, 2022 08:26
@dreamorosi dreamorosi merged commit 471eaca into aws-powertools:main Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metrics This item relates to the Metrics Utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: rename raiseOnEmptyMetrics to throwOnEmptyMetrics
3 participants