Skip to content

Updated pmd priority levels #288

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

Merged
merged 7 commits into from
Sep 21, 2017
Merged

Conversation

krukru
Copy link
Contributor

@krukru krukru commented Jun 19, 2017

PMD uses values 1 to 5 for assigning priority, 1 being the most urgent task and 5 least urgent.

1 - Change absolutely required. Behavior is critically broken/buggy.
2 - Change highly recommended. Behavior is quite likely to be broken/buggy.
3 - Change recommended. Behavior is confusing, perhaps buggy, and/or against standards/best practices.
4 - Change optional. Behavior is not likely to be buggy, but more just flies in the face of standards/style/good taste.
5 - Change highly optional. Nice to have, such as a consistent naming policy for package/class/fields...

See http://pmd.sourceforge.net/pmd-5.0.5/rule-guidelines.html

I think that changing the values here will have no other effect on the application, but will result in a correct pmd file.

@UFOMelkor
Copy link
Member

Could be a BC break …

@krukru
Copy link
Contributor Author

krukru commented Jun 20, 2017

People who are using standard PMD processing tools are having issues with this. Info level messages have a priority of 0, which is undefined (jenkins reports them as critical) and critical issues (currently at priority 4) will be marked as info-level warnings.

People who noticed this and made custom adjustments will have BC broken, but all they need to do is drop their custom changes and use standardised PMD parsing tools

I don't think you need to worry about breaking BC here, since the feature was originally bugged - but it does need to be documented. I will update the PR with a changelog

@krukru
Copy link
Contributor Author

krukru commented Jun 20, 2017

I couldn't find if you are already keeping track of your changes in a changelog, so I created one. I tried sticking to http://keepachangelog.com/en/1.0.0/ since it seems to be getting some good traction lately. I also populated the changelog with changes from previous releases (just for 2.x). What do you think of it?

@UFOMelkor
Copy link
Member

LGTM 👍
Anyway we need to wait for @Halleck45 to have a look at it, because he is the only one with merge access.

@krukru
Copy link
Contributor Author

krukru commented Aug 23, 2017

Hi there, it's been some time now. No commits from the maintainer. Is the project still alive?

@theofidry
Copy link
Contributor

Last commit is from April 12. It's not that old for an open source project. And although the tool is not perfect, it is relatively stable and does a decent job.

@krukru
Copy link
Contributor Author

krukru commented Aug 23, 2017

While I do agree that the tool is stable at it's current state, the fact that there are 10 open pull requests without even a comment from the maintainer in the last 4 months raises concerns.

@theofidry
Copy link
Contributor

It raises valid concerns, but it's not dramatic easier. It's hard to be always very responsive for Open Source :) I do think having more maintainers would help though.

@Halleck45
Copy link
Collaborator

Hi everybody,

we're back :) @UFOMelkor that's way my fault, I tought you've the write access on this repository :/

@krukru , can you remove the changelog.md file from your PR (that's the subject of another pull request)

@krukru
Copy link
Contributor Author

krukru commented Sep 21, 2017

Removed the changelog. Do you want a separate PR for it or will you take care of the changelog?

@niconoe-
Copy link
Contributor

I think changelog must be part of a new PR. I'm merging this one.
@Halleck45 I'll let you take care of the changelog for previous versions, and I can handle the changes between 2.2.0 and the future 2.3.0

@niconoe- niconoe- merged commit f75fa9b into phpmetrics:master Sep 21, 2017
@krukru krukru deleted the pmd-priority-rewrite branch September 22, 2017 09:58
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.

5 participants