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.

Sign up for GitHub

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

Jump to bottom

fix: make notification as local variable #328

Closed
wants to merge 1 commit into from
Closed

fix: make notification as local variable #328

wants to merge 1 commit into from

Conversation

singhvikash11
Copy link
Member

@singhvikash11 singhvikash11 commented Nov 17, 2022 •
edited
Loading

fix #329

@vercel
Copy link

vercel bot commented Nov 17, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
guardian ✅ Ready (Inspect) Visit Preview Nov 17, 2022 at 10:12AM (UTC)

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3487309879

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 74.88%
Totals Coverage Status
Change from base Build 3476997011: 0.0%
Covered Lines: 6108
Relevant Lines: 8157
💛 - Coveralls

@mabdh
Copy link
Member

mabdh commented Nov 17, 2022

Have we tested this? Or could we test this flow to make sure this is a correct solution?

@mabdh
Copy link
Member

mabdh commented Nov 17, 2022

This should be a fix instead a chore right?

@rahmatrhd rahmatrhd changed the title chore: make notification as local variable fix: make notification as local variable Nov 21, 2022
@rahmatrhd
Copy link
Member

can you also share how this changes could fix the issue? wanted to know why the previous code causing the issue 🙏

@bsushmith
Copy link
Member

This PR is no longer needed.

@bsushmith bsushmith closed this Dec 1, 2022
@bsushmith bsushmith deleted the fix_variable_scope branch December 1, 2022 04:11
@ravisuhag
Copy link
Member

ravisuhag commented Dec 7, 2022 •
edited
Loading

@bsushmith Is issue #329 solved in another PR or that still remain open? cc @rahmatrhd

@bsushmith
Copy link
Member

@ravisuhag It is still open - We haven't aligned on when we want to deprecate the older configs (which were present for backward compatibility)
comment - #329 (comment)

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

@ravisuhag ravisuhag Awaiting requested review from ravisuhag ravisuhag is a code owner

@rahmatrhd rahmatrhd Awaiting requested review from rahmatrhd rahmatrhd is a code owner

@AkarshSatija AkarshSatija Awaiting requested review from AkarshSatija AkarshSatija is a code owner

@mabdh mabdh Awaiting requested review from mabdh mabdh is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Getting 3 times slack notification for same resource reminder
6 participants
@singhvikash11 @coveralls @mabdh @rahmatrhd @bsushmith @ravisuhag