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

Hackathon team48.7twh wokier trycatch #15

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Hackathon team48.7twh wokier trycatch #15

wants to merge 3 commits into from

Conversation

wokier
Copy link

@wokier wokier commented Apr 6, 2023

  • Added label 'eco-design' on java:S1696 'Avoid catching NullPointerException.'
  • Improved documentation on tag install tooling

Francois Wauquier added 3 commits April 6, 2023 10:07
@wokier wokier changed the title Hackathon team487twh wokier trycatch Hackathon team48.7twh wokier trycatch Apr 6, 2023
@wokier wokier mentioned this pull request Apr 6, 2023
Closed
@dedece35 dedece35 self-requested a review April 10, 2023 19:50
@dedece35 dedece35 added documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers 🏆 challenge 🏆 Work done during the ecoCode Challenge labels Apr 10, 2023
@dedece35
Copy link
Member

dedece35 commented Apr 10, 2023 •
edited
Loading

Hi @wokier,

first of all, thank you for your PR.

unfortunately, there is already an existing PR about the same subject : #18

this is a PR that purpose is to refactor this tagging system to make it more automatic : start from a markdown file to put tags to native SonarQube rules.
it's a real improvement of this system and I think this is the first thing that we have to integrate before other things.

Sorry but I'm reviewing this PR #18.
Next you could check if your PR is still relevant or not.

On the other hand, actually, rules list in _config.sh file are only here for my tests. this system is not running yet.
For me, we have to do an audit of our existing rules and check if they are already present natively in SonarQube : check issue green-code-initiative/ecoCode#34
Thus, this modified list of rules is not OK because there are some test rules in it.

I modified this PR to make it "draft in progress" waiting for the other PR.

@dedece35 dedece35 marked this pull request as draft April 10, 2023 20:03
dedece35
dedece35 requested changes Apr 10, 2023
View reviewed changes
Copy link
Member

@dedece35 dedece35 left a comment

Choose a reason for hiding this comment

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

as explained, we are waiting for #17 to integrate your modifications

tools/rules_config/README.md

1. change configuration in `_config.sh` file : check requirements above
2. launch `check_tags.sh` to control your rules and tags
3. launch `install_tag.sh` to add custom tag to your rules (change SIMULATION=0 in configration to do real calls)
Copy link
Member

Choose a reason for hiding this comment

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

typo error

tools/rules_config/_config.sh

# list of rule keys that will be updated with new tag
RULES_KEYS=css:S4655,php:S2014,Web:ItemTagNotWithinContainerTagCheck
RULES_KEYS=css:S4655,php:S2014,Web:ItemTagNotWithinContainerTagCheck,java:S1696
Copy link
Member

Choose a reason for hiding this comment

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

please keep only your rule java:S1696 (other rules are only for testing the system

@dedece35 dedece35 added the 👀 👀 review done 👀 👀 review done - waiting for changes label Apr 10, 2023
@dedece35
Copy link
Member

Hi @wokier,

PR #17 has just been merged.
Could you resolve your conflicts regarding that now rules list is inside a new file named SONAR_RULES_REUSED.md in which you must write your rule and an explanation about this inclusion : please check the content of this new file

@dedece35 dedece35 marked this pull request as ready for review October 6, 2023 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dedece35 dedece35 dedece35 requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@wokier wokier

Labels
🏆 challenge 🏆 Work done during the ecoCode Challenge documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers 👀 👀 review done 👀 👀 review done - waiting for changes 👀 👀 waiting commiter 👀 👀
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wokier @dedece35