Skip to content
This repository has been archived by the owner on May 22, 2023. It is now read-only.

Linter integration #44

Closed
wants to merge 4 commits into from
Closed

Linter integration #44

wants to merge 4 commits into from

Conversation

liamzebedee
Copy link
Contributor

@liamzebedee liamzebedee commented Jul 16, 2019

Closes #42

WIP: code doesn't pass JS/SOL lint checks yet wow that was easier than I thought

"openzeppelin-solidity": "^2.2.0"
"eslint-config-keep": "git+https://github.com/keep-network/eslint-config-keep.git#0.1.2",
"openzeppelin-solidity": "^2.2.0",
"solium-config-keep": "git+https://github.com/keep-network/solium-config-keep.git#0.1.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the configs not dev dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh woops! Nice catch

@liamzebedee liamzebedee marked this pull request as ready for review July 16, 2019 14:50
files: '\.js$'
language: script
description: "Checks JS code according to the package's linter configuration"
- id: lint-sol
name: 'lint solidity'
entry: /usr/bin/env bash -c "cd implementation && npm run sol:lint"
entry: /usr/bin/env bash -c "cd solidity && npm run sol:lint"
Copy link
Contributor

Choose a reason for hiding this comment

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

I alllso don't love this, but don't have an immediate fix in mind. Though worth noting, it's usually safer to use sh instead of having to /usr/bin/env bash unless you're relying on bash-specific behavior.

Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

One thing I missed last time: we do need to align our JS style across Thesis, for simplicity's sake, but that will require moving formatting to prettier, and we'll have to see if we can apply that to Solidity.

The biggest thing to flag is that we're using this prettier config:

semi: false
trailingComma: "all"

Looks like prettier supports Solidity via https://github.com/prettier-solidity/prettier-plugin-solidity, but it's maybe early days yet. I'd be okay with leaving Solidity with its own linter-based formatter initially if we find the prettier plugin is problematic.

@nkuba
Copy link
Member

nkuba commented Jan 30, 2020

@Shadowfiend what are your thoughts about "Code-quality rules" in context of using prettier?

Code-quality rules: eg no-unused-vars, no-extra-bind, no-implicit-globals, prefer-promise-reject-errors...

Prettier does nothing to help with those kind of rules. They are also the most important ones provided by linters as they are likely to catch real bugs with your code!

Maybe it would be worth using both prettier and linter?

@Shadowfiend
Copy link
Contributor

We should either redo or most likely close this and supersede with eslint alongside eslint-config-keep 0.3.0, which stacks prettier on top of eslint directly.

We can also use prettier-plugin-solidity as in tbtc to handle solidity as well.

@nkuba
Copy link
Member

nkuba commented Mar 26, 2020

Closing in favour of #328

@nkuba nkuba closed this Mar 26, 2020
@Shadowfiend Shadowfiend deleted the linter-integration branch March 30, 2020 14:47
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.

Solidity and JS linters
3 participants