Skip to content
This repository was archived by the owner on Jan 18, 2023. It is now read-only.

Conversation

asoong
Copy link
Contributor

@asoong asoong commented Jul 27, 2018

No description provided.

@asoong asoong requested a review from a team July 27, 2018 08:46
@coveralls
Copy link

coveralls commented Jul 27, 2018

Pull Request Test Coverage Report for Build 874

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.758%

Totals Coverage Status
Change from base Build 864: 0.0%
Covered Lines: 307
Relevant Lines: 307

💛 - Coveralls


import * as ABIDecoder from 'abi-decoder';
import * as chai from 'chai';
import { SetProtocolUtils as utils } from 'set-protocol-utils';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it best practice to change a class with PascalCase to a variable with camelCase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally think it's more human readable

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so too. What I mean is should utils be Utils since it's a class

Copy link
Contributor

Choose a reason for hiding this comment

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

The edge case is when you need to use static methods with the non-static methods.

I think a cleaner approach is to capitalize so we avoid something like this:

import { SetProtocolUtils as utils }  from 'set-protocol-utils';

const utilities = new utils(web3);

and instead have something like this

import { SetProtocolUtils as Utils }  from 'set-protocol-utils';

const utils = new Utils(web3);

Makes the instance delineated from the parent class.

Lmk if that makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

@asoong asoong force-pushed the alex/update_utils branch from 7392895 to 4e6b9b3 Compare July 27, 2018 22:40
@asoong asoong merged commit 37b1535 into master Jul 27, 2018
@asoong asoong deleted the alex/update_utils branch July 27, 2018 23:24
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.

4 participants