Xpring ESLint - Round 3 #282

0xCLARITY · 2020-05-18T21:24:39Z

High Level Overview of Change

Avoid classes with only static members
Avoid negative if conditions
Disallow parameter properties for classes
Consistently return stuff from functions that return
Follow member-ordering for class members
Avoid magic numbers

Context of Change

3rd (and almost final) step of bringing xpring-common-js onto the new lint config.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Test Plan

npm run test is all still green.

codecov · 2020-05-18T21:26:10Z

Codecov Report

Merging #282 into master will increase coverage by 0.52%.
The diff coverage is 73.17%.

@@            Coverage Diff             @@
##           master     #282      +/-   ##
==========================================
+ Coverage   78.97%   79.50%   +0.52%     
==========================================
  Files           6        6              
  Lines         195      200       +5     
  Branches       56       55       -1     
==========================================
+ Hits          154      159       +5     
  Misses         21       21              
  Partials       20       20

Impacted Files	Coverage Δ
src/XRP/serializer.ts	`66.66% <46.66%> (+0.49%)`	⬆️
src/PayID/pay-id-utils.ts	`83.33% <70.00%> (+0.98%)`	⬆️
src/Common/utils.ts	`100.00% <100.00%> (ø)`
src/PayID/pay-id-components.ts	`66.66% <100.00%> (ø)`
src/XRP/signer.ts	`66.66% <100.00%> (+1.96%)`	⬆️
src/XRP/wallet.ts	`87.71% <100.00%> (+0.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cb0019...0ed8e67. Read the comment docs.

0xCLARITY · 2020-05-18T21:29:24Z

src/Common/utils.ts

@@ -24,7 +24,7 @@ export interface ClassicAddress {
  test: boolean
 }

-abstract class Utils {
+const utils = {


Change from a class to an object that holds functions.

This is the proper fix for the no-extraneous-class rule. I did ask about using abstract classes for this problem, but the consensus seems to be to just use EcmaScript modules.

It's a little odd that we're exporting an object as opposed to the individual functions, but this keeps us consistent with the old way of exporting the class.

0xCLARITY · 2020-05-18T21:31:04Z

src/Common/utils.ts

@@ -85,13 +85,13 @@ abstract class Utils {
    }

    // Xpring Common JS's API takes a string|undefined while the underlying address library wants string|false.
-    const shimTagParameter = tag !== undefined ? tag : false
+    const shimTagParameter = tag ?? false


There's an ESLint rule against if(negative) {...} else {...}, because there's usually a better way to write it (like here).

0xCLARITY · 2020-05-18T21:34:32Z

src/PayID/pay-id-components.ts

-  ) {}
+  public constructor(host: string, path: string) {
+    this.host = host
+    this.path = path


There's a rule that disallows declaring class properties in the constructor. I'm ok with disabling this rule for now, but since some properties might not be declarable in the constructor, it makes sense to be consistent.

I'm happy to use lint rules if that's what folks like doing.

The only reason I could think of not liking this style is that you forget to assign a variable. However, this appears to not be the case, since the following errors:

public constructor(host: string, path: string) { this.host = host }

Yells taht I didn't initialize path.

0xCLARITY · 2020-05-18T21:35:31Z

src/PayID/pay-id-utils.ts

-      return
+  parsePayID(payID: string): PayIdComponents | undefined {
+    if (!this.isASCII(payID)) {
+      return undefined


The consistent-return rule requires being explicit with your return statements if your function contains a return statement that returns something other than undefined (like this function with return new PayIdComponents().

woo. I actually prefer this.

0xCLARITY · 2020-05-18T21:37:08Z

src/XRP/wallet.ts

+   * @param privateKey - The given private key for the wallet.
+   * @param test - Whether the address is for use on a test network, defaults to `false`.
+   */
+  public constructor(publicKey: string, privateKey: string, test = false) {


Moved the constructor because the member-ordering rule defines an order that class members should appear in (and constructors come before any methods).

0xCLARITY · 2020-05-18T21:37:37Z

test/Common/utils.test.ts

@@ -162,13 +163,14 @@ describe('utils', function (): void {
      classicAddress!.address,
      'rU6K7V3Po4snVhBBaU29sesqs2qTQJWDw1',
    )
-    assert.strictEqual(classicAddress!.tag, 12345)
+    assert.strictEqual(classicAddress!.tag, tag)


Added a variable for this to avoid the no-magic-numbers rule. This seems reasonable because the GIVEN comment includes the tag

0xCLARITY · 2020-05-18T21:38:26Z

tsconfig.json

-  "include": [
-    "src/**/*"
-  ]
+  "include": ["src/**/*"]


Moved the src/types directory to @types. I think this is more conventional, but let me know if there's a strong reason to have custom declaration files in the src directory.

I did this because I need to disable a rule for declaration files which I do for all files in @types/**/*.d.ts right now.

keefertaylor

This looks good to me, pending that it builds in Xpring4J and XpringKit. I'm verifying that now.

keefertaylor · 2020-05-19T13:52:15Z

Verified this works in XpringKit and Xpring4J.

I'm going to avoid merging anything so that you can get this PR landed cleanly and I'll rebase the upstream PRs for any issues. Are there further linter PRs to go or is everything enabled?

keefertaylor

Forgot to actually hit approve.

0xCLARITY · 2020-05-19T14:32:35Z

There is probably one more linter PR to go.

The big blocker here is that tsdoc does not support the @see tag yet. I'm working on that here:
microsoft/tsdoc#236

The only other violations are things like this:

/Users/hbergren/Documents/xpring-eng/xpring-common-js/src/XRP/serializer.ts
   45:3   warning  Method 'transactionToJSON' has too many lines (83). Maximum allowed is 50       max-lines-per-function
   45:20  warning  Method 'transactionToJSON' has a complexity of 14. Maximum allowed is 8         complexity
   45:20  warning  Method 'transactionToJSON' has too many statements (39). Maximum allowed is 10  max-statements
  135:16  warning  Method 'paymentToJSON' has too many statements (22). Maximum allowed is 10      max-statements

/Users/hbergren/Documents/xpring-eng/xpring-common-js/test/XRP/serializer.test.ts
  51:1  warning  Function 'makeTransaction' has too many parameters (7). Maximum allowed is 4    max-params
  51:1  warning  Function 'makeTransaction' has too many statements (34). Maximum allowed is 20  max-statements

/Users/hbergren/Documents/xpring-eng/xpring-common-js/test/XRP/signer.test.ts
  24:24  warning  Maximum number of dependencies (8) exceeded                   import/max-dependencies
  27:14  warning  Function has too many statements (39). Maximum allowed is 20  max-statements

/Users/hbergren/Documents/xpring-eng/xpring-common-js/test/XRP/wallet.test.ts
  1:1  warning  File has too many lines (291). Maximum allowed is 250                           max-lines

So, if the @see tag gets taken care of, I either need to do a quick refactor pass to make those functions less complex / take fewer parameters, or I need to add project-specific overrides into the xpring-common-js/.eslintrc.js file.

keefertaylor · 2020-05-19T14:34:00Z

Oh sweet.

I'm pretty open to what you'd recommend as function length / parameter limits. I don't love it as an absolute heuristic, but I'm fine to try it out and see if it encourages me to have better behavior.

0xCLARITY · 2020-05-19T14:45:44Z

I'm pretty open to what you'd recommend as function length / parameter limits. I don't love it as an absolute heuristic, but I'm fine to try it out and see if it encourages me to have better behavior.

Yeah, the config right now are just my best guesses. Super happy to amend them based on feedback, especially as we roll out this linting config to more projects.

In general, my thinking is:

A function is much easier to understand if it fits on a single screen without scrolling (line-length)
Cyclomatic complexity (branching, conditionals, etc) should be kept to a minimum for understandability. I think right now I have it set to 8, which I read somewhere was a threshold for "hard to understand", but it seems the actual standard is 10
max-statements usually goes along with complexity and line-length
max-parens is 4 because that's a lower limit for how much state a person can hold in their head at once. Beyond that it seems reasonable to require a single object parameter, and have a TypeScript interface that defines the object.

amiecorso

This is great! I'm fine with all of these rules - I agree that the function complexity/length might be a bit more subjective, but I think everything here is objectively better. Only exception might be prohibiting parameter properties, which I don't think are too confusing. Though explicit really usually is better so I can get behind it.

0xCLARITY added 3 commits May 18, 2020 17:15

@types

225b24c

no-magic-numbers

89a1931

wip

5f6f253

0xCLARITY commented May 18, 2020

View reviewed changes

0xCLARITY marked this pull request as ready for review May 18, 2020 21:50

0xCLARITY requested review from amiecorso and keefertaylor as code owners May 18, 2020 21:50

keefertaylor reviewed May 19, 2020

View reviewed changes

Merge branch 'master' into xpring-eslint-3

0ed8e67

keefertaylor self-requested a review May 19, 2020 14:30

keefertaylor approved these changes May 19, 2020

View reviewed changes

amiecorso approved these changes May 19, 2020

View reviewed changes

0xCLARITY merged commit 4fc8689 into master May 20, 2020

0xCLARITY deleted the xpring-eslint-3 branch May 20, 2020 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xpring ESLint - Round 3 #282

Xpring ESLint - Round 3 #282

0xCLARITY commented May 18, 2020 •

edited

Loading

codecov bot commented May 18, 2020 •

edited

Loading

0xCLARITY May 18, 2020

0xCLARITY May 18, 2020

0xCLARITY May 18, 2020

keefertaylor May 19, 2020

keefertaylor May 19, 2020

0xCLARITY May 18, 2020

keefertaylor May 19, 2020

0xCLARITY May 18, 2020

0xCLARITY May 18, 2020

0xCLARITY May 18, 2020 •

edited

Loading

keefertaylor left a comment

keefertaylor commented May 19, 2020

keefertaylor left a comment

0xCLARITY commented May 19, 2020

keefertaylor commented May 19, 2020

0xCLARITY commented May 19, 2020

amiecorso left a comment

Xpring ESLint - Round 3 #282

Xpring ESLint - Round 3 #282

Conversation

0xCLARITY commented May 18, 2020 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

Test Plan

codecov bot commented May 18, 2020 • edited Loading

Codecov Report

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

0xCLARITY May 18, 2020 • edited Loading

Choose a reason for hiding this comment

keefertaylor left a comment

Choose a reason for hiding this comment

keefertaylor commented May 19, 2020

keefertaylor left a comment

Choose a reason for hiding this comment

0xCLARITY commented May 19, 2020

keefertaylor commented May 19, 2020

0xCLARITY commented May 19, 2020

amiecorso left a comment

Choose a reason for hiding this comment

0xCLARITY commented May 18, 2020 •

edited

Loading

codecov bot commented May 18, 2020 •

edited

Loading

0xCLARITY May 18, 2020 •

edited

Loading