Skip to content

REQ: add option to ignore import statements (php) #179

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

Closed
Gompje opened this issue Apr 12, 2017 · 9 comments
Closed

REQ: add option to ignore import statements (php) #179

Gompje opened this issue Apr 12, 2017 · 9 comments

Comments

@Gompje
Copy link

Gompje commented Apr 12, 2017

I'm using code climate on a huge Laravel/AngularJS app. So far I'm impressed with the code quality analysis; it's close to 'what I know already'.

However it would be nice to have an option in the duplication engine that allows certain statements to be exact dupes: the namespace and use statements. I have several events defined, for clarity in separate files atm and they all have the same top lines:

namespace App\Events;
 
use Illuminate\Broadcasting\Channel;
use Illuminate\Queue\SerializesModels;
use Illuminate\Broadcasting\PrivateChannel;

This results in a Identical code found in 10 other locations (mass = 57). I'm only just adding these events, there will be at least 5x more. I prefer to keep them in separate files.
Making the treshold higher would mean not catching does nasty little dupes.

Maybe it's possible to add something like 'ignore namespace and use statements' or a more general 'ignore lines starting with ' or even an 'ignore '.

While I realise that I just can ignore these dupe reports, I don't like the clutter it causes and the fact that I always will have to remember to ignore these. They don't help the overall ratings either - they are important for code confidence for myself, our team and our customers.

Or did I miss something in the docs?

@zenspider
Copy link
Contributor

Hi hi... Can you provide me with enough files for a real reproduction? I don't code in PHP so I don't know any of the idioms or how PHP chooses to parse it (yet). I don't need any of your business logic/IP... just enough to look and feel like real PHP that reproduces this error.

I'm working on a structural pattern matching system and want to get standard language idioms filtered automatically, but also allow the user to add their own. (I think... not fully concrete on this yet)

@wfleming wfleming removed their assignment Apr 12, 2017
@zenspider
Copy link
Contributor

Also, the code above with a single basic function only has a mass of 17... so it isn't the use statements themselves that are causing the problem, but the whole file. As it stands, the AST looks like:

s(:ast,
 s(:namespace,
  s(:name),
  s(:use, s(:use_use, s(:name))),
  s(:use, s(:use_use, s(:name))),
  s(:use, s(:use_use, s(:name))),
  s(:function, s(:return, s(:binary_op_plus, s(:lnumber), s(:lnumber))))))

So, maybe the whole files really ARE structurally similar?

@zenspider
Copy link
Contributor

One thing I do NOT want to do (yet?) is to do filtering as a form of modification of the sexp. Right now I think it is best to record everything and just filter what is reported. As such, the code example above IS structurally similar and would be reported because there is no real structural difference between the two files I'm testing against. If the functions were different enough (or different counts of functions or whatever) then they wouldn't get reported.

@Gompje
Copy link
Author

Gompje commented Apr 13, 2017

Sure!

Basically they are just classes in PHP.

The namespace is what you would expect, and would be the same for many classes.

The use keyword is just like import, basically saying 'hey this class is using these and these classes in such and such namespace'. There can be 0 or n import statements.

Example of a simple class in PHP:

<?php

namespace App\Events;

use Illuminate\Broadcasting\Channel;
use Illuminate\Queue\SerializesModels;
use Illuminate\Broadcasting\PrivateChannel;
use Illuminate\Broadcasting\PresenceChannel;
use Illuminate\Broadcasting\InteractsWithSockets;
use Illuminate\Contracts\Broadcasting\ShouldBroadcast;

class SomethingHasHappened
{
    use InteractsWithSockets, SerializesModels;
	
	public $var1;
	public $var2;
	
	/**
	 * Create a new event instance.
	 *
	 * @param $var1
	 * @param $var2
	 *
	 */
	public function __construct($var1,$var2)
	{
		$this->var1 = $var1;
                $this->var2 = $var2;
	}
	
	/**
	 * Get the channels the event should broadcast on.
	 *
	 * @return Channel|array
	 */
	public function broadcastOn()
	{
		return new PrivateChannel('channel-name');
	}
}

Does this help?

@Gompje
Copy link
Author

Gompje commented Apr 13, 2017

Thinking now maybe some kind of level would help, like tagging these kind of minor duplication issues as minor. and then filter them out in the codeclimate UI. This way they are still logged and we can tackle them during some the lows ;)

but like I said I'm a total n00b - maybe this is already possible?

@zenspider
Copy link
Contributor

Not possible, unfortunately. All issues are "errors" at this point. I wish that were not the case so I could submit "info" or "statistics" issues to the engine.

@zenspider
Copy link
Contributor

@Gompje so... are the other 9 files basically the exact same structure? The imports themselves are not the problem... I have stuff in the works that will let them drop off the radar, but it sounds like you have legitimate structural similarities in your 10 files.

@zenspider
Copy link
Contributor

See #190 for my proposed solution to this.

@zenspider
Copy link
Contributor

I believe this is done and in production. Please close if it works for you.

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

No branches or pull requests

3 participants