Skip to content

Annoying Lint PR #2#47

Merged
drwpow merged 1 commit intomainfrom
fix-lint
Apr 1, 2021
Merged

Annoying Lint PR #2#47
drwpow merged 1 commit intomainfrom
fix-lint

Conversation

@drwpow
Copy link
Member

@drwpow drwpow commented Mar 31, 2021

Changes

Enforces the "no-shadow": "error" rule, and also adds several missing JSDoc comments.

You‘ll notice there are still a lot of any warnings in the code. This is where we can negotiate whether or not this is helpful. If it helps us pay down debt (I’ve personally always found a way around any, except in the case of external libraries), let‘s keep it, but if not, let’s ditch it.

The goal is to improve code quality. Which should annoy us a little bit, but shouldn‘t just be noise, either.

Testing

  • Tests are passing
  • Tests updated where necessary

Docs

  • Docs / READMEs updated
  • Code comments added where helpful

'@typescript-eslint/no-empty-function': 'off',
'no-shadow': 'warn',
'prettier/prettier': 'error',
'no-shadow': 'error',
Copy link
Member Author

Choose a reason for hiding this comment

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

The real win here. This can be a pretty nasty/hard-to-spot bug, and I’m a big fan of this lint rule.

@@ -0,0 +1 @@
src/parser/parse/**/*.ts
Copy link
Member Author

Choose a reason for hiding this comment

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

I just disabled linting for code we didn‘t write 🤷🏻


const defn = components[componentName];
const fileUrl = new URL(defn.specifier, filename);
const fileUrl = new URL(defn.specifier, importUrl);
Copy link
Member Author

Choose a reason for hiding this comment

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

no-shadow

for (const match of matches.reverse()) {
const name = match[1];
appendImports(name, filename, astroConfig);
for (const foundImport of matches.reverse()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

no-shadow

}
for (const match of matches.reverse()) {
const name = match[1];
for (const astroComponent of matches.reverse()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

no-shadow

if (!lastValue) continue;
selectors.push({ start, end: isEnd ? n + 1 : n, value: lastValue });
start = n + 1;
{
Copy link
Member Author

Choose a reason for hiding this comment

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

no-shadow

function formatErrorForBrowser(err: Error) {
// TODO make this pretty.
return error.toString();
return err.toString();
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a pretty good find for no-shadow! We had a param with the same name as a function.

@FredKSchott
Copy link
Member

FredKSchott commented Mar 31, 2021

Yea, I don't know if I'm fully on board with "any is always a lint error/warning". For example, I've been pulling my hair out trying to type the AST walking enter/exit functions.

But, if everyones on board with things like type AstNode = any then I'm +1 on treating direct any usage as a lint error.

@drwpow
Copy link
Member Author

drwpow commented Apr 1, 2021

Just disabled the any warnings for now. You‘ll notice that non-null assertions still warn, because those can be a code smell for unhandled cases. Or bad types. But same deal–if those are unavoidable then we can disable that too.

@drwpow drwpow merged commit c26c244 into main Apr 1, 2021
@drwpow drwpow deleted the fix-lint branch April 1, 2021 16:25
ematipico pushed a commit that referenced this pull request Feb 7, 2025
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.

2 participants