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

fix: explicit return statements in KnowledgePanels.pm #7082

Merged
merged 1 commit into from
Jul 20, 2022

Conversation

dipietroR
Copy link
Contributor

Used Perl::Critic to look for functions without return statements. I'm planning on making more PRs similar to this (for all of the other files) and then adding [Subroutines::RequireFinalReturn] to .perlcriticrc

@dipietroR dipietroR requested a review from a team as a code owner July 18, 2022 19:55
@github-actions github-actions bot added the 📖 Knowledge Panels https://wiki.openfoodfacts.org/Knowledge_panels label Jul 18, 2022
@sonarcloud
Copy link

sonarcloud bot commented Jul 18, 2022

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
No Duplication information No Duplication information

@dipietroR dipietroR changed the title fix: return statements to KnowledgePanels.pm fix: explicit return statements in KnowledgePanels.pm Jul 18, 2022
@stephanegigandet
Copy link
Contributor

For reference, here is the rule: https://metacpan.org/pod/Perl::Critic::Policy::Subroutines::RequireFinalReturn

I'm not sure if adding boilerplate return statement adds a lot of value, but I don't have a strong opinion against it.

@dipietroR
Copy link
Contributor Author

I'm not sure if adding boilerplate return statement adds a lot of value, but I don't have a strong opinion against it.

I saw that it was being done in ded3dec so I assumed that we would like to add it in. Of course, if you don't want it done, I could stop and focus on a different rule that we may want added?

@stephanegigandet
Copy link
Contributor

I saw that it was being done in ded3dec so I assumed that we would like to add it in. Of course, if you don't want it done, I could stop and focus on a different rule that we may want added?

Let's implement this rule as you have already done it for many files, as did @Ban3 . I'll approve all the related PRs and we can merge them.

But for the future rules to be added, let's discuss them first to see which ones we think will bring the most value. Could you for instance start an issue on GitHub to list a few new rules you think would be good candidates, with maybe 1 or 2 examples of the issues they would trigger in our code? Then we can review them and discuss the benefits.

Copy link
Contributor

@stephanegigandet stephanegigandet 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!

@stephanegigandet stephanegigandet merged commit c1c901e into main Jul 20, 2022
@stephanegigandet stephanegigandet deleted the Knowledge_Panels-return-statement branch July 20, 2022 08:08
LandonPattison pushed a commit that referenced this pull request Jul 24, 2022
fix: return statements to KnowledgePanels.pm
LandonPattison pushed a commit that referenced this pull request Jul 25, 2022
fix: return statements to KnowledgePanels.pm
LandonPattison pushed a commit that referenced this pull request Jul 25, 2022
fix: return statements to KnowledgePanels.pm
LandonPattison pushed a commit that referenced this pull request Jul 25, 2022
fix: return statements to KnowledgePanels.pm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📖 Knowledge Panels https://wiki.openfoodfacts.org/Knowledge_panels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants