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

Misc cleanup #165

Merged
merged 1 commit into from
May 1, 2023
Merged

Misc cleanup #165

merged 1 commit into from
May 1, 2023

Conversation

irahopkinson
Copy link
Contributor

@irahopkinson irahopkinson commented May 1, 2023

  • remove irrelevant comment in pre-commit hook
  • don't need npx in npm scripts
  • prettier tsconfig.json and launch.json
  • add new to all throw Error to be correct

This change is Reviewable

- remove irrelevant comment in pre-commit hook
- don't need `npx` in npm scripts
- prettier `tsconfig.json` and `launch.json`
- add `new` to all `throw Error` to be correct
@irahopkinson irahopkinson enabled auto-merge (squash) May 1, 2023 03:09
Copy link
Contributor

@rolfheij-sil rolfheij-sil left a comment

Choose a reason for hiding this comment

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

Looks good. One small question about the pre-commit file

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)


.husky/pre-commit line 6 at r1 (raw file):

echo "Running Lint check..."

# need to generate the .d.ts files for the extensions in the extensions/dist folder

Have we already created a task in the backlog for this?

Copy link
Contributor Author

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @irahopkinson)


.husky/pre-commit line 6 at r1 (raw file):

Previously, rolfheij-sil wrote…

Have we already created a task in the backlog for this?

I don't know and didn't create this comment but it's already on main (you would need to ask TJ since he created it). I just removed the comment below it that is no longer relevant. The line used to be npx lint-staged -q (which had the -q option). It now calls the lint:staged script in package.json so the comment is no longer relevant.

@irahopkinson irahopkinson merged commit 9eda4be into main May 1, 2023
@irahopkinson irahopkinson deleted the cleanup branch May 1, 2023 09:57
Copy link
Contributor

@rolfheij-sil rolfheij-sil left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions


.husky/pre-commit line 7 at r1 (raw file):


# need to generate the .d.ts files for the extensions in the extensions/dist folder
# -q enables quiet mode to show nothing unless it has an error

I wish we could put comments in the npm scripts :( would it be worth investigating a good way to do this? Both of these comments are irrelevant really. Both are now embedded in lint:staged. But I just didn't know what to do about them, so I left them.


src/extension-host/services/extension.service.ts line 180 at r1 (raw file):

  // eslint-disable-next-line no-global-assign
  globalThis.WebSocket = function WebSocketForbidden() {
    throw new Error('Cannot use WebSocket!');

Maybe we should make a code style guideline about this?

@irahopkinson
Copy link
Contributor Author

Good Idea. I've added it to the wiki for now but there is a rule for this so I've also added it to the list of todos in the Style Guide Evaluation doc.

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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.

3 participants