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

Fix "Code sniff" step in CI and other updates #92

Merged
merged 12 commits into from
Aug 13, 2021
Merged

Conversation

jabxjab
Copy link
Contributor

@jabxjab jabxjab commented Aug 13, 2021

No description provided.

@jabxjab jabxjab changed the title Fix "Code sniff" step in CI Fix "Code sniff" step in CI and other updates Aug 13, 2021
];
}

parent::__construct($options);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to respect parent's constructor call

public function __construct(LoggerChannelFactoryInterface $loggerChannelFactory, PantheonGuzzle $pantheonGuzzle) {
$this->logger = $loggerChannelFactory->get('PantheonSolr');
$this->client = $pantheonGuzzle;
public function __construct(LoggerChannelFactoryInterface $logger_channel_factory, PantheonGuzzle $pantheon_guzzle_client) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per Drupal's var name best practices, all var and arg names are in snake case, while class properties are in camelcase.

foreach ($files as $filename => $file_contents) {
try {
$response = $this->uploadSchemaFile($filename, $file_contents);
$logFunction = in_array($response->getStatusCode(), [200, 201, 202, 203]) ? 'notice' : 'error';
Copy link
Contributor Author

@jabxjab jabxjab Aug 13, 2021

Choose a reason for hiding this comment

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

This check is not needed because Guzzle throws exceptions on non-200 statuses

$toReturn[] = $message;
$this->getLogger()->{$logFunction}($message);
$output[] = $message;
$this->logger->debug($message);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like "'File: %s, Status code: %d - %s'" is a debug, so logging this as a debug log

* @return \Psr\Http\Client\ClientInterface
* Psr 18 Client, typically PantheonGuzzle instance.
*/
public function getClient(): PSR18Interface {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing client in order to incapsulate and use ->client directly

* @return \Psr\Log\LoggerInterface
* PSR Logger interface.
*/
public function getLogger(): LoggerInterface {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this in favor of LoggerChannelTrait (and setting up in the constructor)

}

/**
* Get verbosity.
*
* @return bool
* Wheither or not to turn on long debugging.
* Whether or not to turn on long debugging.
*/
protected function isVerbose(): bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not used as of now, needs to be deleted.

@@ -35,8 +34,11 @@ jobs:
mkdir -p /root/.ssh && echo "StrictHostKeyChecking no" >> "/root/.ssh/config"
terminus auth:login -n --machine-token="$TERMINUS_TOKEN"

- name: Composer install
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to run phpcs, composer installation neede

- name: Code sniff
run: ./vendor/bin/phpcs --report=full --standard=vendor/drupal/coder/coder_sniffer/Drupal . --ignore=vendor
run: composer run-script code:lint
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better to use already defined composer run-script

public function postSchema(string $server_id = 'pantheon_solr8') {
public function postSchema(?string $server_id = NULL) {
if (!$server_id) {
$server_id = PantheonSolrConnector::getDefaultEndpoint();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove hardcoded value in favor of the canonical one

@@ -273,7 +278,7 @@ public function viewSchema(string $filename = 'schema.xml') {
if (!$schemaPoster instanceof SchemaPoster) {
throw new \Exception('Cant get Schema Poster class. Something is wrong with the container.');
}
$currentSchema = $schemaPoster->viewSchema('pantheon_solr8', $filename);
$currentSchema = $schemaPoster->viewSchema($filename);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

server_id is not needed (not used) in viewSchema() anymore, so removing

*/
public function getSolrFiles(string $server_id = 'pantheon_solr8') {
protected function getSolrFiles(string $server_id) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

making protected since not used outside


$this->setLogger($loggerChannel->get('PantheonGuzzle'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing logger since not used

$guzzle = new PantheonGuzzle($loggerChannel);
$queryResult = $guzzle->getSolrClient()->update($query);
$this->assertTrue($queryResult->getResponse->getStatusCode() == 200);
$pantheon_guzzle = new PantheonGuzzle();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix for the test

@@ -20,8 +20,10 @@
},
"autoload-dev": {
"psr-4": {
"Drupal\\search_api_pantheon\\" : "./src",
"Drupal\\search_api_pantheon\\tests\\" : "./tests"
"Drupal\\search_api_pantheon\\" : "src/",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this phpunit won't find search_api_* related resources on my local

./setup-d8-repo.sh
./enable-modules.sh
./verify-solr.sh
# - name: Tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

commenting out since does not work currently

@jabxjab jabxjab requested a review from stovak August 13, 2021 14:39
@stovak stovak merged commit 29dff6b into 8.x Aug 13, 2021
@stovak stovak deleted the feature/ci-code-validation branch August 13, 2021 15:12
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

Successfully merging this pull request may close these issues.

None yet

2 participants