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

Integrate eslint with eslint-config-standard #385

Merged
merged 6 commits into from
Jun 21, 2022
Merged

Integrate eslint with eslint-config-standard #385

merged 6 commits into from
Jun 21, 2022

Conversation

kaisalmen
Copy link
Collaborator

@kaisalmen kaisalmen commented Jun 20, 2022

I have based this branch on your branch, because otherwise the changes are obsolete again soon and would need to be redone. Therefore the target branch is currently also use-monaco-vscode-api

@CGNonofr I used eslint-config-standard, but I made some adjustments:

  • Indent is 4 instead of 2
  • I favor semi-colons
  • no-dupe-class-member is off
  • no-redeclare, no-useless-constructor and no-void have been reduced to warn (you should check if the code should or could be changed or if eslint-disable should be set instead and we can then remove the rule reduction)

This closes #366

@kaisalmen kaisalmen requested a review from CGNonofr June 20, 2022 09:34
Base automatically changed from use-monaco-vscode-api to main June 20, 2022 15:26
Copy link
Collaborator

@CGNonofr CGNonofr left a comment

Choose a reason for hiding this comment

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

Good job!

.eslintrc.js Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
.github/workflows/actions.yml Outdated Show resolved Hide resolved
packages/client/src/monaco-language-client.ts Outdated Show resolved Hide resolved
@CGNonofr
Copy link
Collaborator

Btw, you can use the header/header rule to ensure all files have the proper header

@kaisalmen
Copy link
Collaborator Author

I implemented the review comments and rebased the branch. Now, the build will fail because vscode-ws-jsonrpc will have lint related errors. I will fix this afternoon, I think.

@kaisalmen
Copy link
Collaborator Author

I have now enabled eslint globally for all packages and thanks for mentioning header/header. I have now integrated it.

@kaisalmen
Copy link
Collaborator Author

Build is green. Everything is in. If you agree with the latest changes this is good to go, I think.

Make disposable.ts in both packages identical for now
@kaisalmen
Copy link
Collaborator Author

I will produce new dev releases once this is merged. Sounds good?

@CGNonofr
Copy link
Collaborator

CGNonofr commented Jun 21, 2022

I will produce new dev releases once this is merged. Sounds good?

Sounds good! What will we wait for before releasing a final version?

@CGNonofr CGNonofr self-requested a review June 21, 2022 14:39
Copy link
Collaborator

@CGNonofr CGNonofr left a comment

Choose a reason for hiding this comment

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

LGTM

@kaisalmen kaisalmen merged commit 47b4e50 into main Jun 21, 2022
@kaisalmen kaisalmen deleted the eslint branch June 21, 2022 14:41
@kaisalmen
Copy link
Collaborator Author

Sounds good! What will we wait for before releasing a final version?

Actually a good question. We don't really have official beta tester. And a major version change should not immediately cripple correctly configured projects depending on here.

@CGNonofr
Copy link
Collaborator

CGNonofr commented Jun 21, 2022

It seems pretty safe to release a major version to me, and release fixes later, or we won't get any feedback

I'm already testing the new vscode-api approach for more than a week, and everything else is just refactors, it can't break things, can it?

@kaisalmen
Copy link
Collaborator Author

it can't break things, can it?

Let's go

@kaisalmen
Copy link
Collaborator Author

@CGNonofr
Copy link
Collaborator

@kaisalmen
Copy link
Collaborator Author

We should archive the repos of @codingame/monaco-jsonrpc and vscode-ws-jsonrpc and add links in the README that link to here.

@CGNonofr
Copy link
Collaborator

We should archive the repos of @codingame/monaco-jsonrpc and vscode-ws-jsonrpc and add links in the README that link to here.

Will do it on my side

@CGNonofr
Copy link
Collaborator

done!

@kaisalmen
Copy link
Collaborator Author

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.

Add eslint configuration
2 participants