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

chore: replace standard and snazzy with neostandard #3485

Merged
merged 10 commits into from
Aug 21, 2024
Merged

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Aug 18, 2024

Migrate from standard and snazzyy to neostandard by @voxpelli . Also starting to lint typescript files.

To make it easier to review:

  • First commit is just setting up eslint with neostandard instead of standard and snazzy, + configure for our project
  • Second commit is with automatic fixing via --fix flag
  • Third commit is manually fixing linting errors

@Uzlopak Uzlopak changed the title Use neostandard chore: replace standard and snazzy with neostandard Aug 18, 2024
Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

Why? If we are going this way I'd rather go with eslint + prettier.

eslint.config.js Outdated Show resolved Hide resolved
}),
{
rules: {
'@stylistic/comma-dangle': ['error', {

Choose a reason for hiding this comment

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

Would love to get feedback on neostandard/neostandard#79 to make this unnecessary 🙏

Choose a reason for hiding this comment

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

If you want, you can update to 0.11.3 now and this override will no longer be needed (though having it will enforce no dangling commas if that's desireable)

eslint.config.js Outdated
exports: 'never',
functions: 'never'
}],
'@typescript-eslint/no-redeclare': 'off',

Choose a reason for hiding this comment

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

Was this a particular issue with TS-files? Maybe it's something we should tweak in neostandard itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this basically happens if you have something like

export interface CacheStorage {
  match (request: RequestInfo, options?: MultiCacheQueryOptions): Promise<Response | undefined>,
  has (cacheName: string): Promise<boolean>,
  open (cacheName: string): Promise<Cache>,
  delete (cacheName: string): Promise<boolean>,
  keys (): Promise<string[]>
}

declare const CacheStorage: {
  prototype: CacheStorage
  new(): CacheStorage
}

Because the interface CacheStorage exists declaring the const CacheStorage results in a no-redeclare linting error

eslint.config.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

Why? If we are going this way I'd rather go with eslint + prettier.

prettier is incompatible with standard so it would be impossible to do backports. neostandard is just an eslint config and backward compatible with standard.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Aug 19, 2024

I worked now together with @voxpelli regarding this PR, because the linting was quite slow (~10s). Disabling jsx to avoid running react related linting saved 2 s. Also activated eslint cache makes it run significantly faster.

So the first run is slow but consecutive runs will have the same perf as before.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Aug 19, 2024

Well. I think the PR is now mature enough for the merge.

So if @ronag 's is ok with it, then we could merge it :).

Copy link
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

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

lgtm

@Uzlopak Uzlopak merged commit 26004bf into main Aug 21, 2024
38 checks passed
@Uzlopak Uzlopak deleted the use-neostandard branch August 21, 2024 10:10
Copy link
Contributor

The backport to v6.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-v6.x v6.x
# Navigate to the new working tree
cd .worktrees/backport-v6.x
# Create a new branch
git switch --create backport-3485-to-v6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 26004bfccaead3d6ec143c03a09dc036cea2c2ae
# Push it to GitHub
git push --set-upstream origin backport-3485-to-v6.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-v6.x

Then, create a pull request where the base branch is v6.x and the compare/head branch is backport-3485-to-v6.x.

@github-actions github-actions bot mentioned this pull request Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants