[apex,vf] Fix #1648 - Remove CodeClimate dependency #1702
Conversation
Generated by 🚫 Danger |
oowekyala
changed the title
There was a problem hiding this comment.
Thanks for the contribution @rsoesemann. Overall I'm happy with the change, I think rule properties should be used only to configure the rule's behaviour, not expose metadata.
So if I sum up my understanding of this changeset:
- Properties
cc_categories,cc_remediation_points_multiplier,cc_block_highlightingcan no longer be overridden by a user (since they don't exist anymore, any user overriding them would get a "Property does not exist" error)- This should be documented explicitly in the release notes, not just with a link to the issue (in this PR)
- Deprecations should be ported to master in another PR
- The CodeClimate renderer now only outputs default values
- This affects the codeclimate engine: https://github.com/codeclimate/codeclimate-pmd/blob/5455b68ad66a3e211b9a39320e69cf4e5cd83aa7/src/main.groovy#L28
- So if they want to display non-default values, they'd have to maintain themselves a mapping of rules to codeclimate properties
- Maybe in the future we can expose a built-in way to associate metadata with rules, for consumption by external engines. Ideally that would be done outside of the rule definitions to avoid mixing optional metadata with behaviour properties. But that's definitely outside the scope of this PR
| <ruleset xmlns="http://pmd.sourceforge.net/ruleset/2.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="Default ruleset used by the CodeClimate Engine for Salesforce.com Apex" xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd"> | ||
| <description>Default ruleset used by the Code Climate Engine for Salesforce.com Apex</description> | ||
| <ruleset xmlns="http://pmd.sourceforge.net/ruleset/2.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="Default ruleset for Salesforce.com Apex" xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd"> | ||
| <description>Default ruleset for Salesforce.com Apex</description> |
There was a problem hiding this comment.
This ruleset should probably be renamed quickstart.xml for consistency with the Java one. Not in this PR though. I'll open an issue to track that.
There was a problem hiding this comment.
No need for an extra issue. I'll do that in my corrections.
There was a problem hiding this comment.
Regarding your above comments:
- This PR brings PMD where it was before CodeClimate wise. I never heard that anyone in the Salesforce world uses it so its fair to clean up the mess I made. I have no interest in improving anything Code Climate related. Let that do others...
- I will add the additional consequences to the release notes doc.
- Porting to master. Sound right will do it when this is merged. Maybe you can show me some tricks (via Gitter) how to do that ;-)
pmd-core/src/main/java/net/sourceforge/pmd/renderers/CodeClimateRule.java
Show resolved
Hide resolved
|
Comments to your overall comment:
|
|
I accidentally "resolved" your comment by renaming the description of the ruleset. I don't want to do it because it is not a quickstart but the default that every starter should use. There are not that many and no controversial rules so its simpler to just say: Bäm thats the default. Use it! |
| <rule-property name="skipTestMethodUnderscores">true</rule-property> | ||
| <expected-problems>0</expected-problems> |
There was a problem hiding this comment.
I don't understand this change: Why is this necessary, or did we change the rule-test schema?
There was a problem hiding this comment.
The builds failed here because the order was wrong. I have no clue why this didn’t fail in the past but I fixed it for this pr
There was a problem hiding this comment.
We seem to run into this problem: junit-team/junit5#987
The tests for MethodNamingConventions are not executed at all on the master branch, due to the XML invalid problem.
We won't merge from 7.0.0 -> master, but only from master -> 7.0.0.
Ideally, we'll do this change on master first before we merge this PR to 7.0.0. |
|
Btw, it seems like Apex support was only added in November 2018 to code climate (see codeclimate/codeclimate-pmd@5455b68)... for Java, I assume, they just use the default category/remediation points... I see, your plugin https://docs.codeclimate.com/docs/apexmetrics is deprecated in favor of the more general https://docs.codeclimate.com/v1.0/docs/pmd |
|
Ok as I understand @adangel I will add a second PR for master PR first before this will be merged. |
|
I see we deprecated |
|
See also: #1648 (comment) But true, as soon we are going to remove the code climate specific properties, the category and remediation points will fall back to default.... |
No description provided.