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

[TypeScript] Lint TypeScipt code #736

Merged
merged 2 commits into from
May 6, 2020
Merged

[TypeScript] Lint TypeScipt code #736

merged 2 commits into from
May 6, 2020

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented May 6, 2020

Currently the typescript code is not being linted in the CI. This PR adds some basic recommended typescript linting rules and fixes to code so that the linting succeeds.

In order to fix an import lint on importing truffle-assertions, I added some typings for the module.

Test Plan

CI - linting should succeed.

@nlordell nlordell requested a review from a team May 6, 2020 09:14
Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

Very cool. It goes without saying that I didn't review all the ts changes, as I assume they were automated.

@nlordell
Copy link
Contributor Author

nlordell commented May 6, 2020

Very cool. It goes without saying that I didn't review all the ts changes, as I assume they were automated.

I had to add type return types everywhere. The semi-colon stuff was automated.

I split this out into #737 and #738 which each contain some non-automatic/mechanical changes to appease the linter.

nlordell added a commit that referenced this pull request May 6, 2020
This PR includes changes that were a part of #736 and that require some special attention.

This PR moves the generated truffle typings to the `build/` directory, so we can use the `types/` directory for manually created type definitions.

Added typings for the `truffle-assertions` npm package for use in the truffle tests.

### Test Plan

CI!
Nicholas Rodrigues Lordello added 2 commits May 6, 2020 15:14
@nlordell nlordell merged commit 0763337 into master May 6, 2020
@nlordell nlordell deleted the strict-linting branch May 6, 2020 13:31
nlordell added a commit that referenced this pull request May 6, 2020
This PR adds orderbook JSON types

This change was included as part of #736 but I think it deserves some special attention.

### Test Plan

Unit tests validate serialization and deserialization.

### Commit History

* add orderbook JSON types

* separate JSON types
Comment on lines -3 to 4
// eslint-disable-next-line no-undef
const BUILD_DIR = path.join(__dirname, "build", "contracts")
// eslint-disable-next-line no-undef
const NETWORKS_FILE_PATH = path.join(__dirname, "networks.json")
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird... why where these even here. Looks like they were perfectly defined..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the ESLint env was incorrectly setup for this file.

Copy link
Contributor

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

I would say that the semis could have been removed separately so I could actually detect what else was changed.

@nlordell
Copy link
Contributor Author

nlordell commented May 6, 2020

I would say that the semis could have been removed separately so I could actually detect what else was changed.

The issue is that CI would fail as the formatting would be incorrect. I suggest filtering by .json and .js in the GitHub diff UI.

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.

4 participants