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

remove the empty function #17

Closed
wants to merge 7 commits into from
Closed

Conversation

Srijan25
Copy link

image
correct this click code
image
remove the empty constructor

Copy link
Contributor

deepsource-io bot commented Oct 12, 2024

Here's the code health analysis summary for commits ff6fe9f..7150530. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource JavaScript LogoJavaScript✅ Success
🎯 5 occurences resolved
View Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

@nmarulo nmarulo self-assigned this Oct 12, 2024
@nmarulo
Copy link
Member

nmarulo commented Oct 12, 2024

@Srijan25
I have left you a comment in the src/app/modules/shopping-list/pages/add-products-shopping-list/add-products-shopping-list.component.ts file. Regarding the other two changes, which deepsource issues refer to these two file changes?

If they are not related to an issue, they do not need to be made in this issue. That refactoring can be left for another issue. Anyway, as it is already done, we leave it as it is.

@nmarulo nmarulo linked an issue Oct 12, 2024 that may be closed by this pull request
Copy link
Author

@Srijan25 Srijan25 left a comment

Choose a reason for hiding this comment

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

yeah sorry i didn't realise it was stuck in infinite loop i fixed it

Copy link
Author

@Srijan25 Srijan25 left a comment

Choose a reason for hiding this comment

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

yeah i remove that logic because i think its unnecessary

Copy link
Author

@Srijan25 Srijan25 left a comment

Choose a reason for hiding this comment

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

resolved

@Srijan25 Srijan25 requested a review from nmarulo October 14, 2024 04:57
Copy link
Author

@Srijan25 Srijan25 left a comment

Choose a reason for hiding this comment

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

can you check once

@nmarulo
Copy link
Member

nmarulo commented Oct 14, 2024

Check if you have uploaded all the changes and test it before, please.

image

@nmarulo
Copy link
Member

nmarulo commented Oct 14, 2024

Read the recommendations given by deepsource, I believe that in this case it is ok to do so and there is no need to replace or delete it.

https://app.deepsource.com/gh/despensa-app/despensa-web/issue/JS-0321/occurrences?listindex=0

For example in this PR #19, although I do not accept it for another reason, the solution is also valid for me.

image

Sometimes the solution may be simpler than you think.

@nmarulo nmarulo closed this Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detected empty functions JS-0321
2 participants