codeclimate-duplication icon indicating copy to clipboard operation
codeclimate-duplication copied to clipboard

React's propTypes often marked as duplicates

Open jdelStrother opened this issue 9 years ago • 11 comments

Hi - a common source of false-positives in our CC duplication results comes from React's propType structures. For example:

  propTypes: {
    playBtnState: React.PropTypes.string.isRequired,
    height: React.PropTypes.number.isRequired,
    drawBack: React.PropTypes.bool
  },

is a 'duplicate' of:

  propTypes: {
    clipStore: React.PropTypes.object.isRequired,
    firstClipId: React.PropTypes.number.isRequired,
    includeDetails: React.PropTypes.bool
  },

Can the engine be improved to avoid this, or is there anything we can do in our codebase that would help? Excluding each fingerprint on a case-by-case basis is getting tiresome.

jdelStrother avatar Jan 28 '16 09:01 jdelStrother

@jdelStrother Thank you for the feedback! We are working to improve our Duplication engine's analysis of JavaScript and make results more savvy.

I'm going to tag this issue as Enhancement and add its consideration to our roadmap.

In the meantime, one workaround might be to to experiment with upping the mass threshold for JavaScript duplication in your .codeclimate.yml config settings:

engines:
  duplication:
    enabled: true
    config:
      languages:
         javascript:
           mass_threshold: 50

Our default for JavaScript is 40. Increasing the threshold should make the detection of duplication less sensitive.

ABaldwinHunter avatar Feb 01 '16 17:02 ABaldwinHunter

Very similar situation occurs when checking Ember imports:

import Ember from 'ember';
import OnboardingButton from 'app/mixins/onboarding-button';

const { Component } = Ember;

is a duplication of something like this ie.:

import Ember from 'ember';
import UnauthenticatedRouteMixin from 'ember-simple-auth/mixins/unauthenticated-route-mixin';

const { Route } = Ember;

export default Route.extend(UnauthenticatedRouteMixin);

This affects overall score a lot :(

netes avatar Mar 24 '16 09:03 netes

@netes @jdelStrother Sorry for the frustration here. Right now the duplication algorithm effectively compares syntax nodes based on both what kind of expression they represent, as well as the contents of those expressions. When the kinds of the expressions are the same, but the contents vary (which looks to be the case here), the engine considers that "similar" duplication to distinguish it from "identical" duplication.

The detection of "similar" duplication like your case is one that, while it can turn up helpful results in many cases, is unfortunately also prone to these kinds of false-positives that are easy to distinguish as a human but not easy to make a decision about within an algorithm. We are continuing to investigate ways of improving the algorithm for these kinds of cases, but need to be careful that in avoiding false-positives we don't swing too far and stop returning useful instances of similar code. So for the time being we don't consider this behavior a bug since this is the expected behavior of Flay, the underlying tool this engine uses for detecting duplication.

There are several workarounds available to you in the meantime:

  1. You can ignore particular issues using their fingerprints. This is a good approach if these kinds of reports are an infrequent annoyance for you.

  2. As Ashley noted above, you can tune the mass threshold of duplicate code that the engine will report to suite your preferences. This will effect both similar & identical issues. (Ruby's default mass threshold is 40.)

    engines:
      duplication:
        enabled: true
        config:
          javascript:
            mass_threshold: 50
    
  3. If you would prefer not to see reports of similar code at all, you can disable that check entirely. If you do this, the engine will only report instances of code it sees as identical.

    engines:
      duplication:
        enabled: true
        checks:
          Similar code:
            enabled: false
    

@netes If you use any of these workarounds to exclude these issues, they will no longer affect your repo's grade.

wfleming avatar Mar 31 '16 19:03 wfleming

See #190 for my proposed solution to this.

zenspider avatar Jun 14 '17 23:06 zenspider

This ticket is a bit stale, I just want to note that the work from #190 is now part of the stable channel, and users should still use that instead of beta.

We are not counting imports as duplication by default. Excluding propTypes: declarations is not curently being done by default, but for users who are still seeing that happen frequently you can configure your own pattens.

wfleming avatar Apr 03 '18 16:04 wfleming

Sorry to bring back a stale issue but I'm running into this with

constructor(props){
    super(props)

being marked as duplicate and I can't fathom how to filter this out.

RyanMacG avatar Jul 25 '18 13:07 RyanMacG

It looks like assignments were recently made to team members, but this is something that makes the code duplication rule very difficult to manage in React projects. Really excited to see this feature come in.

kylemh avatar Jul 30 '18 04:07 kylemh

To those who landed here and saw @wfleming's suggestion about configuring filter patterns but didn't want to do the legwork of running dump_ast (like I did), this worked for me in terms of fixing propTypes defined like this:

Component.propTypes = {
  foo: PropTypes.string
}

The following configuration works:

plugins:
  duplication:
    config:
      languages:
        javascript:
          filters:
            - "s(:property, s(:Identifier, :propTypes))"

schuylr avatar Jan 15 '20 16:01 schuylr

That configuration is not working for me. I get it working using:

version: "2"
plugins:
  duplication:
    config:
      languages:
        javascript:
          filters:
            - "(object (Identifier PropTypes))"

This also works for me:

version: "2"
plugins:
  duplication:
    config:
      languages:
        javascript:
          filters:
            - "(property (Identifier propTypes))"

I changed it based on this quote:

The internal representation (which is ruby) is different from the pattern language (which is lisp-like), so first we need to convert s(: to ( and remove all commas and colons:

from: https://github.com/codeclimate/codeclimate-duplication

rodolfo3 avatar Apr 30 '20 17:04 rodolfo3

But is still catching repetitions on things like PropTypes.func or PropTypes.shape. I'm testing it using:

/opt/codeclimate/bin/codeclimate analyze --dev -e duplication -f text <path>

I set dump_ast: true on .codeclimate.yml. Thare is no PropType or propType on generate AST. But still have shape, number, etc.

rodolfo3 avatar Apr 30 '20 18:04 rodolfo3

Facing similar kind of issue, in React js as well. While calling a util component in 2 different components file with different parameter it detects it as similar code.

deveshbajaj59 avatar Aug 26 '20 05:08 deveshbajaj59