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.

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

React's propTypes often marked as duplicates #89

Open
jdelStrother opened this issue Jan 28, 2016 · 11 comments
Open

React's propTypes often marked as duplicates #89

jdelStrother opened this issue Jan 28, 2016 · 11 comments
Assignees

Comments

@jdelStrother
Copy link

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.

@ABaldwinHunter
Copy link
Contributor

@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.

@netes
Copy link

netes commented Mar 24, 2016

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 :(

@wfleming
Copy link
Contributor

@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.

@zenspider
Copy link
Contributor

See #190 for my proposed solution to this.

@wfleming
Copy link
Contributor

wfleming commented Apr 3, 2018

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.

@RyanMacG
Copy link

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.

@kylemh
Copy link

kylemh commented Jul 30, 2018

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.

@schuylr
Copy link

schuylr commented Jan 15, 2020

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))"

@rodolfo3
Copy link

rodolfo3 commented Apr 30, 2020

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
Copy link

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.

@deveshbajaj59
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests