Skip to content
This repository has been archived by the owner on Apr 7, 2024. It is now read-only.

Adding basic linting. #47

Merged
merged 4 commits into from
Oct 10, 2018
Merged

Adding basic linting. #47

merged 4 commits into from
Oct 10, 2018

Conversation

fmizzell
Copy link
Contributor

@fmizzell fmizzell commented Sep 16, 2018

Connects #13
QA

  • Run dktl phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,info /var/www/dkan/modules/dkan/<some_module>
  • If you want to fix some of the issues, run dktl phpcbf --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,info /var/www/dkan/modules/dkan/<some_module>
  • Run dktl test:lint dkan/modules/dkan/dkan_datastore dkan/modules/dkan/dkan_dataset
  • Run dktl test:lint-fix dkan/modules/dkan/dkan_datastore dkan/modules/dkan/dkan_dataset

@dafeder
Copy link
Member

dafeder commented Sep 19, 2018

@fmizzell the way I imagined it when I wrote the original ticket was the options except for the path should be default, and it should default to current dir so you should be able to just type dktl test:lint and get something.

@fmizzell
Copy link
Contributor Author

@dafeder The problem with the approach you are proposing is that it makes it really hard to get out of the constraints you are imposing. For example if I have a library that I wan to lint that uses PSR2, it would be very hard for us to return that power to the user.

@dafeder
Copy link
Member

dafeder commented Sep 19, 2018

@fmizzell can't we have the arguments get default values and then you just set them differently if you want to?

@fmizzell
Copy link
Contributor Author

The final decision is to make the commands work with some defaults, but also expose phpcs directly.

Copy link
Member

@dafeder dafeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fmizzell made some suggestions but would accept "let's do another PR for cleanup," lmk how you want to handle.

*/
public function phpcbf(array $args) {
$dktl_dir = Util::getDktlDirectory();
$phpcs_command = "{$dktl_dir}/vendor/bin/phpcbf";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this $phpcbf_command?

$dktl_dir = Util::getDktlDirectory();
$project_dir = Util::getProjectDirectory();

$phpcs_command = "{$dktl_dir}/vendor/bin/phpcs";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe create a class property or get method for phpcs_command so we don't have this redundant code?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...although actually, wouldn't it make more sense to re-use $this->phpcs?

@dafeder dafeder merged commit 9f99388 into master Oct 10, 2018
@fmizzell fmizzell deleted the 13-lint branch November 2, 2018 19:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants