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

Lint entire codebase #4156

Closed
wants to merge 4 commits into from
Closed

Lint entire codebase #4156

wants to merge 4 commits into from

Conversation

joeizang
Copy link
Collaborator

Give the whole project a once over manually linting every file.

@joeizang joeizang added the ready-to-review PR ready to review label Oct 20, 2023
@joeizang joeizang self-assigned this Oct 20, 2023
@netlify
Copy link

netlify bot commented Oct 20, 2023

Deploy Preview for remixproject ready!

Name Link
🔨 Latest commit 3f99288
🔍 Latest deploy log https://app.netlify.com/sites/remixproject/deploys/6536da763765b200089e080a
😎 Deploy Preview https://deploy-preview-4156--remixproject.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@bunsenstraat
Copy link
Collaborator

are the rules used in linting this also in the lint config? meaning, does it checked during the lint step in CI? otherwise it wouldn't make sense.

@joeizang
Copy link
Collaborator Author

are the rules used in linting this also in the lint config? meaning, does it checked during the lint step in CI? otherwise it wouldn't make sense.

basically yes. It follows the eslint rules you created. I just applied them per file manually.

use: ["source-map-loader"],
enforce: "pre"
use: ['source-map-loader'],
enforce: 'pre',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if we should use comma for last item in JSON . See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Trailing_commas

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This isn't a json file. its a js file


config.watchOptions = {
ignored: /node_modules/
ignored: /node_modules/,
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

}
return res.join(' ');
case 'ContractDefinition':
return undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

semicolon should be removed

return undefined;

case 'FunctionDefinition': {
const { kind, name } = item;
Copy link
Collaborator

Choose a reason for hiding this comment

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

semicolon should be removed


case 'FunctionDefinition': {
const { kind, name } = item;
const params = item.parameters.parameters;
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here


case 'ModifierDefinition': {
const params = item.parameters.parameters;
return `modifier ${item.name}(${params.map(formatVariable).join(', ')})`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

semicolon should be removed. Please check all such places

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved

@bunsenstraat
Copy link
Collaborator

are the rules used in linting this also in the lint config? meaning, does it checked during the lint step in CI? otherwise it wouldn't make sense.

basically yes. It follows the eslint rules you created. I just applied them per file manually.

then why would linting pass in the previous codebase? if the rule was in place.. aren't we mixing prettier and linting.

@joeizang
Copy link
Collaborator Author

are the rules used in linting this also in the lint config? meaning, does it checked during the lint step in CI? otherwise it wouldn't make sense.

basically yes. It follows the eslint rules you created. I just applied them per file manually.

then why would linting pass in the previous codebase? if the rule was in place.. aren't we mixing prettier and linting.

@bunsenstraat I think we aren't. The thing is prettier ensures the files are formatted with 2 spaces with 2 or 3 rules for semi colons, & commas. So I just did the 2 space thing for code files that were missed (somehow) and fix any existing semi colons.

@joeizang joeizang requested a review from Aniket-Engg October 24, 2023 10:02
_paq.push(['setTrackerUrl', u + 'matomo.php'])
_paq.push(['setSiteId', domains[window.location.hostname]])
var d = document, g = d.createElement('script'), s = d.getElementsByTagName('script')[0]
g.async = true g.src = '//cdn.matomo.cloud/ethereumfoundation.matomo.cloud/matomo.js' s.parentNode.insertBefore(g,s)
Copy link
Collaborator

Choose a reason for hiding this comment

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

see this is an example of where problems come with this method...

@bunsenstraat
Copy link
Collaborator

bunsenstraat commented Oct 25, 2023

What I suggest we do is:
we runt lint fix on each nx project and leave the other files untouched.
and we sync the prettier file to reflect the rules exactly as we discussed earlier.
I don't feel confident merging it like this. the removal ; fix breaks some things if applied carelessly.
for example:
console.log('test')
(window as any).doSomething()
would cause errors without a ; in between.
also errors happened with:
g.async = true; g.src = '//cdn.matomo.cloud/ethereumfoundation.matomo.cloud/matomo.js' s.parentNode.insertBefore(g,s)
g.async = true g.src = '//cdn.matomo.cloud/ethereumfoundation.matomo.cloud/matomo.js' s.parentNode.insertBefore(g,s)
so this method of fixing is not reliable.

@joeizang joeizang closed this Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-review PR ready to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants