Introduce Required Parameter Sniff #76

moorscode · 2018-03-30T20:41:04Z

Initially only checking update_option for the autoload parameter to be set.
This is an optional parameter, but we want to make sure it is set with intent.

As this class is based upon the AbstractFunctionParameterSniff-class from WPCS, some class-aliases need to be loaded to make it work.
These are being provided in the autoload-bootstrap.php file.

To make sure the tests also know where to find them, a test bootstrap file had to be added as well.

@jrfnl do I need to give props for the inspirational files and extended class and if so, how?

Fixes #74

jrfnl · 2018-03-30T20:47:32Z

do I need to give props for the inspirational files and extended class and if so, how?

Just saying "Thank you" to me will do ;-)

Other than that, I'll have a look & review the sniff in a little. ~~Before I do - any particular reason not to sniff for add_option as I suggested in the ticket ?~~ Scratch that, just noticed you added a new commit adding it.

moorscode · 2018-03-30T20:53:53Z

I was too wrapped up in getting the context that I didn't even read your comment on the issue until I created the PR.

Have read it now and added commits accordingly 😊

jrfnl

Reviewed. Besides the remarks I've left inline, I'm wondering what the goal is of the checks currently in the sniff.

Is your intention that the param should always be set ?
Or that the param is always set to true/yes ?

The default is true/yes for add_option() and whatever was set when the option was originally added for update_option(), defaulting to true/yes when the option doesn't exist yet.

So what are you really trying to archive with this sniff ?

jrfnl · 2018-03-31T02:14:33Z

Tests/bootstrap.php

+ * This aliasing has to be done before any of the test classes are loaded by the
+ * PHPCS native unit test suite to prevent fatal errors.
+ *
+ * @package WPCS\WordPressCodingStandards


I think this docblock still needs adjusting, credit should be given for the file to WPCS, but the package, link and since tags should be for YoastCS.

jrfnl · 2018-03-31T02:16:33Z

Tests/bootstrap.php

+}
+
+// Load our class aliases.
+require_once dirname( __DIR__ ) . $ds . 'autoload-bootstrap.php';


The path to the WP alias file is different in a test situation, compared to a "run" situation, so loading the ruleset bootstrap here won't work as the paths won't match, so you need to load the alias file directly.

require_once dirname( __DIR__ ) . '/vendor/wp-coding-standards/wpcs/WordPress/PHPCSAliases.php';

In a test situation, YoastCS is the root.
In a "run" situation, YoastCS is installed in the vendor directory of a project.

jrfnl · 2018-03-31T02:18:03Z

Yoast/Sniffs/Files/FileNameSniff.php

@@ -11,7 +11,6 @@

 use PHP_CodeSniffer\Sniffs\Sniff;
 use PHP_CodeSniffer\Files\File;
-use PHP_CodeSniffer\Util\Tokens;


This change does not belong in this PR.

Technically correct. Will it stop you from merging the PR?

It will not ;-)

jrfnl · 2018-03-31T02:20:22Z

Yoast/Sniffs/WP/RequiredParametersSniff.php

+				continue;
+			}
+
+			$message = '%s() is expected to be called with the "%s" argument at #%s.';


This sentence does not read right. Might help to change at #%s to at position %d.

jrfnl · 2018-03-31T02:21:47Z

Yoast/Sniffs/WP/RequiredParametersSniff.php

+				$position,
+			);
+
+			$this->addMessage( $message, $stackPtr, $parameter_args['enforce'], $code, $data, 0 );


$severity is optional and should normally not be set, so please remove it.

jrfnl · 2018-03-31T02:28:32Z

Tests/bootstrap.php

+ *
+ * Load the PHPCS autoloader and the WPCS PHPCS cross-version helpers.
+ *
+ * {@internal We need to load the PHPCS autoloader first, so as to allow their


This comment is not needed in the context of the YoastCS standard as YoastCS does not need the aliasing as such.
It could be changed to refer to the fact that WPCS at this point in time still needs the aliases and that as you extend a WPCS class, it is needed here too.

As a side-note: WPCS will be dropping PHPCS 3.x support by the end of the summer, so at that time, the aliases will be removed from WPCS completely.

jrfnl · 2018-03-31T02:31:37Z

Yoast/Sniffs/WP/RequiredParametersSniff.php

+	/**
+	 * The group name for this group of functions.
+	 *
+	 * Intended to be overruled in the child class.


Comment line is superfluous as this is a child class.

jrfnl · 2018-03-31T02:33:11Z

Yoast/ruleset.xml

@@ -2,6 +2,9 @@
 <ruleset name="Yoast" namespace="YoastCS\Yoast">
 	<description>Yoast Coding Standards</description>

+	<!-- Autoload the autoloaders of the dependencies. -->


I've ran some tests, but I don't think this is actually needed as the ruleset includes the WPCS WordPress ruleset and that ruleset already loads the WPCS autoload. This can be (and should be) removed.

jrfnl · 2018-03-31T02:34:56Z

autoload-bootstrap.php

@@ -0,0 +1,16 @@
+<?php


As per the comment about autoload in the ruleset + the comment about the paths being different in a test situation and needing to include the WPCS alias file directly for a test situation, this whole file isn't necessary.

jrfnl · 2018-03-31T02:39:57Z

autoload-bootstrap.php

+ * @author   Jip Moors
+ */
+
+if ( file_exists( __DIR__ . '/vendor/wimg/php-compatibility/PHPCSAliases.php' ) ) {


Side-note/Teaching moment: the PHPCompatibility alias file would not be needed anyway as - again - their own ruleset loads it.

Basically, the only time when you need to load the alias files of another standard is when you just load sniffs from another standard, not the standard itself.

So, for the QA-WP project, I load them as those rulesets don't include the whole WP standard, but only individual sniffs.
For the unit tests you need to load them as the sniffs are tested individually, independent of the ruleset.

jrfnl · 2018-03-31T02:57:05Z

Oh.. and two more things:

The Tests directory also needs to be added to the gitattributes export-ignore section.
The Travis script needs adjusting as the phpunit command now adds a bootstrap on the command-line which overrules the one you've added.

moorscode · 2018-03-31T13:49:34Z

The main goal of the Sniff is to make sure the autoload is set according to intend. We want to avoid having options autoloaded if they are not needed outside of the admin.

Having the default yes is not wanted for most of the situations where options are used.

@jdevalk has a ticket in progress/in mind where the level of options will be extended with autoload for admin, to make sure we can still autoload everything that is needed efficiently in the backend, but also loading only what is actually needed on the frontend.

moorscode · 2018-03-31T16:23:37Z

I've processed all the feedback.

jrfnl

@moorscode Looking good. Nearly there, though I still have a couple of remarks:

1. Travis script

The Travis script was set up to:

pull in PHPCS at a PHPCS_BRANCH version, set in the matrix, so the unit tests would be run against the minimum and maximum supported PHPCS version.
pull in WPCS and PHPCompatibility for the CS check of the YoastCS code only, so using the master branch was fine.

However, now you need WPCS (not PHPCompatibility) for unit testing the sniffs too.
But the sniffs will normally be installed using Composer, which fixes WPCS at a certain version range, so using the WPCS master branch does not guarantee that the sniffs are unit tested based on the code which the "end-users" will use.

In other words: as the version of WPCS has now become important, the Travis script needs to be changed. It has to pass the exact branch(es) needed for WPCS to the git clone command.

In the future, the matrix might also need to be extended to test against the minimum and maximum supported version of WPCS if more than one version is supported.
At this time, WPCS 0.14.0 and 0.14.1 are supported, but as 0.14.1 contains no significant changes compared to 0.14.0 for the abstract sniff you are using, just testing against 0.14.1 will be fine.

You could also consider switching over to using Composer to install the dependencies, but there are some snakes under the grass with that so if you want, I could pull that change separately later.

2. The parameter should be set

The main goal of the Sniff is to make sure the autoload is set according to intend.

If that's the case, an explicit value of null for the parameter should not be accepted, as the parameter in that case is still not set.

To fix this, you can test the raw value of the parameter, so I would suggest changing line 75 of the sniff:

if ( isset( $parameters[ $position ] ) ) {

to:

if ( isset( $parameters[ $position ] ) && $parameters[ $position ]['raw'] !== 'null' ) {

3. Unit tests

My previous remark And what about when the param has been set to false/ no ? was not intended to say that every single possible value needs to be tested.
I was just looking to see some more variety in the unit tests.

Testing every possible value is just testing the same thing several times.

For this particular group of functions, I think testing each function with each type of value, i.e. no param, null, a boolean and a string, should be sufficient.
Between those test cases, you can then have the suggested variety by having one of the functions use true, the other false etc.

Other

Will you clean up the branch to topical commits ? Or shall I squash on merge ?

jrfnl · 2018-03-31T17:22:28Z

Tests/bootstrap.php

+
+// Get the PHPCS dir from an environment variable.
+$phpcsDir = getenv( 'PHPCS_DIR' );
+$phpcsComposerDir = implode( $ds, array( dirname( __DIR__ ), 'vendor', 'squizlabs', 'php_codesniffer' ) );


The implode logic feels like something which should be abstracted to a helper function as it's used more than once.

Aside from that, it may be useful to set a $phpcsComposerBaseDir = dirname( __DIR__ ); above this line.

jrfnl · 2018-03-31T17:24:11Z

Tests/bootstrap.php

+	}
+}
+else {
+	echo 'Uh oh... can\'t find PHPCS. Are you sure you are using PHPCS 3.x ?


This second part of this line is in WPCS as it still supports both PHPCS 2.x as well as 3.x. It is not needed in YoastCS.

jrfnl · 2018-03-31T17:24:43Z

Tests/bootstrap.php

+}
+
+// Get the WPCS dir from an environment variable.
+$wpcsDir = getenv( 'WPCS_DIR' );


jrfnl · 2018-03-31T17:25:15Z

Tests/bootstrap.php

+	$wpcsDir = realpath( $wpcsDir );
+}
+
+// Try and load the PHPCS autoloader.


PHPCS -> WPCS

jrfnl · 2018-03-31T17:27:14Z

Yoast/Sniffs/Files/FileNameSniff.php

@@ -11,7 +11,6 @@

 use PHP_CodeSniffer\Sniffs\Sniff;
 use PHP_CodeSniffer\Files\File;
-use PHP_CodeSniffer\Util\Tokens;


It will not ;-)

jrfnl · 2018-03-31T17:39:36Z

Yoast/Sniffs/WP/RequiredParametersSniff.php

+				continue;
+			}
+
+			$message = '%s() is expected to be called with the "%s" argument at the %d position.';


And just wondering - is expected to sounds like a warning, not an error, while depending on context you throw either an error or a warning.

Should this phrase be changed depending on the error/warning context to sound stricter ? %s() must be called with the ...

jrfnl · 2018-03-31T17:40:26Z

Yoast/Tests/WP/RequiredParametersUnitTest.inc

+add_option( 'a', 'b', '', 'no' );
+
+// Ok.
+add_option( 'a', 'b', '', null );


null is not a valid value to pass to add_option(). See my remark in my overall review comment.

jrfnl · 2018-03-31T17:48:43Z

Yoast/Sniffs/WP/RequiredParametersSniff.php

+use WordPress\AbstractFunctionParameterSniff;
+
+/**
+ * Verifies that optional parameters are set, when they should be set.


, when they should be set is redundant. These parameters are optional as stated in the first part of the sentence, so there is no should be set, other than that the YoastCS standard wants them to be, which is what the sniffs is all about in the first place.

jrfnl · 2018-03-31T17:49:44Z

Yoast/Sniffs/WP/RequiredParametersSniff.php

+ *
+ * @since   0.6
+ */
+class RequiredParametersSniff extends AbstractFunctionParameterSniff {


Just wondering - would RequiredOptionalParameters better describe what the sniff covers ?

jrfnl · 2018-03-31T17:51:22Z

Tests/bootstrap.php

+/**
+ * Bootstrap file for running the tests.
+ *
+ * Load the PHPCS autoloader and the WPCS PHPCS cross-version helpers.


You asked about giving credit, but don't mention that the bulk of the code in this file comes from/is inspired by WPCS at all now....

moorscode · 2018-04-02T09:45:45Z

Will you clean up the branch to topical commits ? Or shall I squash on merge ?
I would love to clean up the branch, but I'm missing the context on the specific tools and commands to do so at this point.

I'd say a squash would be fine for this one.
I think having a dev-presentation about achieving this would be very nice (perhaps even have it recorded for easy playback).

jrfnl

@moorscode We're very nearly there. Looking good indeed 👍, just some dotting of the i's and crossing of the t's left.

There is just one last issue I'd like to discuss based on the last round of changes:

You currently have one error message with one error code being thrown from this sniff.
This message can be either a warning or an error and the content of the message changes based on two, three different things.

The idea behind the error codes is that each code identifies a unique message. This enables modular disabling/enabling of sniffs and codes, both from the ruleset as well as inline.

The current message does not comply with that as it basically has four different messages folded into one, all using the same error code.

So, what about actually having two different errors and each having two different codes based on their error/warning status ?

I'm thinking along the lines of:

%s() is %s to be called with the "%s" argument at position %d%s. with depending on error/warning status a different error code, something along the lines of ParamRequired/ParamExpected/ParamMissing.
Take your pick or come up with your own better alternative.
Passing null to the "%s" argument of %s() is %s where the last %s would expand to forbidden or discouraged.
For the error code you could then, for instance, use NullForbidden and NullDiscouraged.

Other than that, some practical points:

👍 I will squash the commits. Let's plan that dev session about having a clean & useful git history via Slack.
Would you like me to create a PR to change the Travis file to use Composer ? As per Introduce Required Parameter Sniff #76 (review)
You are rightfully using version 0.6.0 for this sniff, as the introduction of a new sniff should up the version minor.
There have been some PRs merged between the 0.5.0 tag and now. Should 0.5.1 be tagged before this PR is merged ? Or should those other merged PRs be retagged as 0.6.x ?

jrfnl · 2018-04-03T01:36:58Z

Tests/bootstrap.php

+ *
+ * Load the PHPCS autoloader and the WPCS PHPCS cross-version helpers.
+ *
+ * Code has been inspired on the phpcs3-bootstrap.php used in the WordPress Coding Standards project


Nitpick: inspired by or based on. Inspired on is grammatically incorrect.

Nitpick 2: The see keyword/tag in docblocks is intended to be used for FQCN, the @link tag for URLs. And for documentation purposes, linking to the actual file is better as now another dev would still need to dig through the WPCS repo to find the file.

Can you have multiple @link tags in a documentation block? As there already is one referencing to the project itself a couple of lines down.

You can, but more than that, you can use inline @link tags, which in this case would probably be more appropriate. {@link URL link text} and PHPDocumentor will handle it perfectly ;-)

See: https://docs.phpdoc.org/references/phpdoc/tags/link.html

jrfnl · 2018-04-03T01:41:03Z

Yoast/Sniffs/WP/RequiredOptionalParametersSniff.php

+	 *        (string) Function name. => array(
+	 *            (int) Target parameter position, 1-based. => array(
+	 *                'name'    => (string)  The name of the argument that should be set.
+	 *                'enforce' => (boolean) If this should trigger an error or warning.


Documentation for allow_null missing.

jrfnl · 2018-04-03T01:42:40Z

Yoast/Sniffs/WP/RequiredOptionalParametersSniff.php

+				'allow_null' => false,
+			),
+		),
+	); // End $target_functions.


These type of // End comments are not needed/required. Is there a particular reason why you are adding it ?

It was present in the file I copied it from (the abstract class this is extending).

The abstract class doesn't actually have the end tag. I think you may have copied it from the WP.DeprecatedParameters sniff, but that's a different situation as the array there is nearly 200 lines long.

jrfnl · 2018-04-03T01:42:59Z

Yoast/Sniffs/WP/RequiredOptionalParametersSniff.php

+	/**
+	 * Process the parameters of a matched function.
+	 *
+	 * This method has to be made concrete in child classes.


Superfluous comment line: You are in the child class.

jrfnl · 2018-04-03T01:45:29Z

Yoast/Sniffs/WP/RequiredOptionalParametersSniff.php

+	}
+
+	/**
+	 * Determines if the parameter provided is valid, if any.


Grammatically better: "Determines if a valid parameter has been provided."

jrfnl · 2018-04-03T01:46:05Z

Yoast/Sniffs/WP/RequiredOptionalParametersSniff.php

+	 * @param int   $position       The position of the parameter to check.
+	 * @param array $parameter_args The configuration of the function check.
+	 *
+	 * @return bool True if a valid parameter has been given.


➕ False otherwise.

jrfnl · 2018-04-03T01:50:48Z

Yoast/Sniffs/WP/RequiredOptionalParametersSniff.php

+			return false;
+		}
+
+		if ( ! $parameter_args['allow_null'] && $parameters[ $position ]['raw'] === 'null' ) {


Will this check always need to be executed ? In other words: will allow_null always be set ?

jrfnl · 2018-04-03T02:12:43Z

Yoast/Tests/WP/RequiredOptionalParametersUnitTest.php

+use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
+
+/**
+ * Unit test class for the RequiredParameters sniff.


RequiredParameters =>RequiredOptionalParameters

moorscode · 2018-04-09T07:17:23Z

I have split up the error and warning messages to have their own identifiers.
Rephrased the error codes to be more readable and include Null if disallowed.

Examples:

"update_optionOptionalParamAutoloadMissingNullDisallowed"
"add_optionRequiredParamAutoloadMissingNullDisallowed"

jrfnl · 2018-07-26T16:25:17Z

FYI - the Travis script change over to use Composer as discussed in #76 (review), is included in PR #81.

Applicable to the `update_option()` and `add_option()` functions from WordPress. Ensures that options are not autoloaded without a conscious decision.

Because we use WPCS 1.x, which still supports PHPCS 2.x, we need to make sure the WPCS alias file which adds cross-version support for PHPCS 2.x and 3.x is loaded, when running the unit tests. As this introduces a YoastCS PHPUnit bootstrap file, this file should also take case of loading the PHPCS 3.1+ PHPUnit bootstrap which sorts out PHPUnit cross-version compatibility for PHPCS. Once WPCS 2.0 has been released and bumped, this workaround can be removed.

jrfnl

@moorscode I've reviewed the sniff again and left some (minor) inline remarks.

Aside from that, a couple of generic points I'd like to discuss with you when you have the time:

The way the logic is written now, the mental overhead to read the code is quite high.
Even though, the errorcode is now modular, IMO you are trying to do to much with one error.
Why not have two separate checks ? One that the parameter is set and a second that it's not null ? and have separate errors for each.
Both errors and warnings should be solvable by other means that adding an phpcs:ignore comment.
If an option is being added with add_option() with the $autoload parameter set and in another part of the code that same option is updated using update_option() without setting the $autoload parameter - to prevent overruling whatever was set with add_option() -, there is currently no easy way to "fix" this in the code, other than duplicating the $autoload parameter used in the original add_option() call and creating a potential maintenance/debug nightmare where every single add_option() and update_option() call for each option has to be kept in line with each other and if the $autoload is changed for one, it has to be changed in every single update_option() call for that same option.
I think this needs to be thought through a little more before this sniff can be merged.

jrfnl · 2018-11-12T15:03:32Z

Yoast/Tests/WP/RequiredOptionalParametersUnitTest.php

+			22 => 1,
+			24 => 1,
+		);
+	} // end getErrorList()


End comments are not needed.

jrfnl · 2018-11-12T15:04:41Z

Yoast/Tests/WP/RequiredOptionalParametersUnitTest.php

+	/**
+	 * Returns the lines where warnings should occur.
+	 *
+	 * The key of the array should represent the line number and the value


This "long description" can be removed (as it has been in the method above and in all the other test files).

Yoast/Sniffs/WP/RequiredOptionalParametersSniff.php

As the new sniff is based on WPCS, we need to make sure that the sniff works both with the lowest supported WPCS version as well as with the current `develop` branch. This commit adds high/low WPCS versions into the matrix mix.

jrfnl · 2018-11-12T21:30:20Z

Another point which will need looking into - now or later (non-urgent): the unit tests against PHP nightly (7.4) are failing specifically on this sniff.
See: https://travis-ci.org/Yoast/yoastcs/builds/454169892

I'm not sure if this is PHPCS. WPCS or YoastCS related as I haven't looked into it.
Note: both the PHPCS native tests as well as the WPCS native tests currently pass against nightly.

moorscode · 2018-11-14T13:39:53Z

If an option is being added with add_option() with the $autoload parameter set and in another part of the code that same option is updated using update_option() without setting the $autoload parameter - to prevent overruling whatever was set with add_option() -, there is currently no easy way to "fix" this in the code, other than duplicating the $autoload parameter used in the original add_option() call and creating a potential maintenance/debug nightmare where every single add_option() and update_option() call for each option has to be kept in line with each other and if the $autoload is changed for one, it has to be changed in every single update_option() call for that same option.
I think this needs to be thought through a little more before this sniff can be merged.

These update_option and add_option functions are a bit silly looking at the code.

As the update_option calls add_option when it does not exist, and add_option tries to INSERT the option into the table but does an UPDATE when the key already exists..

We have only one usage of add_option in our code and thinking about it I would rather make sure all calls are just update_option requests with the enforced autoload argument.

If the same option is updated on multiple locations the autoload context must be thought about anyway. If we change if something needs to be autoloaded doing a search would be logical to determine where it is actually set/used.

/cc @atimmer

Co-Authored-By: moorscode <jhp.moors@gmail.com>

jrfnl · 2019-02-20T18:04:25Z

@moorscode So.. how about we try and prevent this PR having a one-year anniversary ? Did you have a chance to think over the things we discussed in December ?

jrfnl requested changes Mar 31, 2018

View reviewed changes

jrfnl added the Type: enhancement label Mar 31, 2018

jrfnl added this to the 0.6.x milestone Mar 31, 2018

jrfnl requested changes Mar 31, 2018

View reviewed changes

jrfnl requested changes Apr 3, 2018

View reviewed changes

moorscode changed the base branch from master to develop April 4, 2018 12:57

jrfnl modified the milestones: 1.0.0, Next release Jul 26, 2018

moorscode force-pushed the 74-introduce-required-parameters-sniff branch 2 times, most recently from 8073f26 to b2e087f Compare November 8, 2018 13:52

jrfnl force-pushed the 74-introduce-required-parameters-sniff branch from 46be857 to 4d3bb89 Compare November 12, 2018 16:00

moorscode added 2 commits November 12, 2018 17:02

✨ Introduces the Required Parameters Sniff

23f340a

Applicable to the `update_option()` and `add_option()` functions from WordPress. Ensures that options are not autoloaded without a conscious decision.

jrfnl force-pushed the 74-introduce-required-parameters-sniff branch from 4d3bb89 to ca5fe4e Compare November 12, 2018 16:02

jrfnl reviewed Nov 12, 2018

View reviewed changes

jrfnl force-pushed the 74-introduce-required-parameters-sniff branch from ca5fe4e to 9ca9009 Compare November 12, 2018 20:58

Travis: expand the matrix to test against high/low WPCS

9ca9009

As the new sniff is based on WPCS, we need to make sure that the sniff works both with the lowest supported WPCS version as well as with the current `develop` branch. This commit adds high/low WPCS versions into the matrix mix.

jrfnl and others added 2 commits November 14, 2018 14:40

Update Yoast/Sniffs/WP/RequiredOptionalParametersSniff.php

d25ddf8

Co-Authored-By: moorscode <jhp.moors@gmail.com>

Apply suggestions from code review

6f75bb9

Co-Authored-By: moorscode <jhp.moors@gmail.com>

jrfnl removed this from the 1.2.1 milestone Dec 29, 2018

jrfnl added this to the Future release milestone Dec 29, 2018

jrfnl mentioned this pull request Jan 16, 2019

Update to using WPCS 2.0.0 #118

Merged

jrfnl added the Status: blocked label Jul 13, 2021

jrfnl closed this Sep 22, 2023

jrfnl removed this from the Future release milestone Sep 22, 2023

jrfnl removed the Status: blocked label Sep 22, 2023

Introduce Required Parameter Sniff #76

Introduce Required Parameter Sniff #76

Conversation

moorscode commented Mar 30, 2018

jrfnl commented Mar 30, 2018 • edited

moorscode commented Mar 30, 2018

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Mar 31, 2018 • edited

moorscode commented Mar 31, 2018

moorscode commented Mar 31, 2018

jrfnl left a comment • edited

Choose a reason for hiding this comment

1. Travis script

2. The parameter should be set

3. Unit tests

Other

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moorscode commented Apr 2, 2018

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moorscode commented Apr 9, 2018

jrfnl commented Jul 26, 2018

jrfnl left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Nov 12, 2018

moorscode commented Nov 14, 2018

jrfnl commented Feb 20, 2019

jrfnl commented Mar 30, 2018 •

edited

jrfnl commented Mar 31, 2018 •

edited

jrfnl left a comment •

edited

jrfnl left a comment •

edited