search

ICCM-EU / BOF

Birds of a feather flock together: this is a software that can be used for nominating BOF topics and for voting on them
MIT License
6 stars 7 forks source link

Codacy mentions 556 issues for the BOF software #112

Open tpokorra opened 4 years ago

tpokorra commented 4 years ago

@hjtappe suggested we use the service of codacy, which is free for Open Source projects. You can see the dashboard here: https://app.codacy.com/gh/ICCM-EU/BOF/dashboard

We could keep the issues in mind. I don't want to make a push for fixing the issues right now. But if there are volunteers, go for it!

franc6 commented 4 years ago

Neat! It looks like Codacy relies pretty heavily on PHPMD (https://phpmd.org), which has some questionable defaults, like marking every use of "else" as a problem.

hjtappe commented 4 years ago

My impression is that the red markings make most sense, such as unreachable code, unused variables etc.. The yellow markers can partly not be understood on my side, like the discouraged used of setcookie. I think some checks can be deactivated if we can identify the check which annoys us, especially removing the *.sql files from checking and keeping only HTML, JS and PHP files.

The blue "else" messages are more a hint than a warning to suggest reviewing code complexity, I think. They do not need fixing.

From another project, my experience is that fixing the red warnings makes most sense and is what is highlighted during a merge, but even they can be ignored, but give the reviewer an indiction to check if it is acceptable.

Apart from unused variables (and "print e->message" debug messages which could reach the end-user and spoil the HTML), I did not see a bothering message in this project yet.

hjtappe commented 4 years ago

I have updated the settings a bit (removing e.g. the "else" warning and the discouraged functions warning) and put the SQL files into a analysis blacklist so they do not generate syntax errors any more. Lets see what the next merge / commit shows as remaining errors.

franc6 commented 4 years ago

Should we set up an "organization" that tracks with the GitHub organization? As far as I can tell, there would be no other way to upload coverage reports. I'm not sure yet how useful such coverage reports are, but I'm not sure we can tell without them. :)

franc6 commented 4 years ago

It looks like code coverage reports generally improve the "grade" for each file and the project over-all.

See also pull request #121 for some more discussion items.

@hjtappe If you send me the list of files you added to the blacklist, I can update #121 with that change, too.

tpokorra commented 4 years ago

ups sorry, now I already merged #121. but I guess you can continue your work on that branch, and create a new pull request. Please mark pull requests with WIP in the title, or mark the PR as draft, so I know immediately if it is not ready for merging yet!

tpokorra commented 4 years ago

I see, we now have an organisation: https://app.codacy.com/gh/organizations/ICCM-EU/settings/people @franc6, I have added you to the organization on Github, and after you accept the invitation, I hope you will be member of the organisation in Codacy as well, or we can add you there.

hjtappe commented 4 years ago

@franc6 I have set ignoreFiles to /sql - which ignores all SQL files therein. https://app.codacy.com/gh/ICCM-EU/BOF/settings/ignoreFiles

Please also update the checks enabled in https://app.codacy.com/gh/ICCM-EU/BOF/patterns/list to a useful minimum. I have the gut feeling that we should disable at least some of them, especially in the area of code compatibility or coding style (e.g. the "Mixed double and single quotes." rule) which we don't require towards older or newer versions of PHP or for readability and consistency.

franc6 commented 4 years ago

@tpokorra I'd forgotten about the draft setting. :( I'll try to remember in the future. For now, though, since nothing has been merged since, reverting would be trivial. I also have access to the organization through both Github & Codacy.

@hjtappe Thanks for the info, I have access to it now, too, so I can see what's there more easily. I noticed that both ESLint and JSHint are enabled, which seems a bit redundant to me. Should we just disable all of the style options? I usually prefer "pick something and use it consistently," but I'm flexible. :) For the most part, I've tried to follow whatever style I see in a given file, e.g. gulpfile.js uses semicolons at the ends of statements, while cypress/integration/*.js do not.