Skip to content

improv(commons): add number key type to JSONObject type #1715

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

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

KhurramJalil
Copy link
Contributor

@KhurramJalil KhurramJalil commented Sep 27, 2023

Fixes #1689

Description of your changes

Related issues, RFCs

Issue number: closes #1689

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
  • I have added tests that prove my change is effective and works
  • The PR title follows the conventional commit semantics

Breaking change checklist

Is it a breaking change?: NO

  • 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.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@KhurramJalil KhurramJalil requested a review from a team September 27, 2023 06:06
@boring-cyborg boring-cyborg bot added the commons This item relates to the Commons Utility label Sep 27, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 27, 2023

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #typescript channel on our Powertools for AWS Lambda Discord: Invite link

@pull-request-size pull-request-size bot added the size/XS PR between 0-9 LOC label Sep 27, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@KhurramJalil KhurramJalil changed the title Update utils.ts Update utils.ts to fix issue #1689 Sep 27, 2023
@github-actions
Copy link
Contributor

No related issues found. Please ensure there is an open issue related to this change to avoid significant delays or closure.

@github-actions github-actions bot added do-not-merge This item should not be merged need-issue This PR needs an issue before it can be reviewed/worked on further labels Sep 27, 2023
@dreamorosi
Copy link
Contributor

Hi @KhurramJalil thanks for opening this issue.

The PR is being labeled as do-not-merge, as well as having a failed check due to the following reasons:

  • (do-not-merge) out automation is not detecting any related issue in the PR body. I see that you have linked the issue, however you've done so outside of the expected format, which is why it's failing. For reference, this is the relevant part of the PR template:
### Related issues, RFCs

<!--- 
Add here the number (i.e. #42) to the Github Issue or RFC that is related to this PR.

Don't include any other text, otherwise the Github Issue will not be detected.

Note: If no issue is present the PR might get blocked and not be reviewed.
-->
**Issue number:** 

The issue number should have gone near **Issue number:**, so once filled it'd be like this:

### Related issues, RFCs

<!--- 
Add here the number (i.e. #42) to the Github Issue or RFC that is related to this PR.

Don't include any other text, otherwise the Github Issue will not be detected.

Note: If no issue is present the PR might get blocked and not be reviewed.
-->
-- **Issue number:** 
++ **Issue number:** closes #1689
  • (failed check) the PR title does not follow the expected format/convention. We use conventional commit semantics for PR titles as this allows us to version our project based on the squashed merge commits. The last checkbox that you have marked in the PR body is about this and has a link with examples of allowed keywords. In this case I'd suggest changing the title to something like "improv(commons): add number key type to JSONObject type".

Once the issues above have been fixed I'll be happy to approve & merge the PR 😃

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.

Hi @KhurramJalil thanks again for the PR, please review the comment above and let me know if you need any additional help.

@KhurramJalil KhurramJalil changed the title Update utils.ts to fix issue #1689 improv(commons): add number key type to JSONObject type (Updated utils.ts file to fix issue #1689) Sep 27, 2023
@KhurramJalil
Copy link
Contributor Author

Thanks for your kind review and feedback @dreamorosi. I have taken care of both the feedback points.

  • Included issue number in the right format at the right place
  • Updated the title of the PR as per recommended standard

Hi @KhurramJalil thanks again for the PR, please review the comment above and let me know if you need any additional help.

@dreamorosi dreamorosi removed do-not-merge This item should not be merged need-issue This PR needs an issue before it can be reviewed/worked on further labels Sep 27, 2023
@dreamorosi dreamorosi self-requested a review September 27, 2023 07:26
@github-actions github-actions bot added the enhancement PRs that introduce minor changes, usually to existing features label Sep 27, 2023
@dreamorosi dreamorosi changed the title improv(commons): add number key type to JSONObject type (Updated utils.ts file to fix issue #1689) improv(commons): add number key type to JSONObject type Sep 27, 2023
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.

Thank you for addressing all the comments, I've made a small edit to the title to remove the content of the old one.

Other than that, it's good to go. Congrats on your first PR merged in this project 🎉

@dreamorosi dreamorosi merged commit 9cc7190 into aws-powertools:main Sep 27, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 27, 2023

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

@KhurramJalil KhurramJalil deleted the patch-1 branch September 27, 2023 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commons This item relates to the Commons Utility enhancement PRs that introduce minor changes, usually to existing features size/XS PR between 0-9 LOC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSONObject should support narrowing to [key: number]:JSONValue
2 participants