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

Update vscode engine and test runners #18

Merged
merged 3 commits into from
Mar 24, 2024

Conversation

dylanbeattie
Copy link
Contributor

This updates the vscode engine to 1.87.0, the TypeScript compiler target to ES2022, and replaces the old Mocha test runner with the new VS Code test runner.

(Presentation Buddy is awesome and I'm working on a new feature for it, which needed a working test runner to figure out some parsing things. I'm happy to build off my own fork if this PR is going to break your publishing flow or anything but figured I'd open the PR and see what you thought!)

This updates the vscode engine to 1.87.0, the TypeScript compiler target to ES2022, and replaces the old Mocha test runner with the new VS Code test runner.
@mauricedb
Copy link
Owner

Cool, I really appreciate the pr. I am a little bit busy at the moment but will review and get back to you as soon as possible.

@dylanbeattie
Copy link
Contributor Author

So the actual feature I propose to add is implemented here:

dylanbeattie#1

(I haven't opened a PR for this one yet because until the tooling PR is merged, the diffs between my branch and the upstream master are a bit of a mess)

Notes on what it adds (and how it works) are in the README - take a look, I'd love to know whether you think this is a useful addition to Presentation Buddy or if you'd prefer I spin it off as a new extension.

Thanks 😊

Copy link
Owner

@mauricedb mauricedb left a comment

Choose a reason for hiding this comment

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

I am fine with merging this PR except for one thing. I would prefer to use NPM as the package manager instead or Yarn V1.

// to report the results back to the caller. When the tests are finished, return
// a possible error to the callback or null if none.
// //
// // PLEASE DO NOT MODIFY / DELETE UNLESS YOU KNOW WHAT YOU ARE DOING
Copy link
Owner

Choose a reason for hiding this comment

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

🧹 Looks like this file can be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can update the PR over the weekend to use npm. The existing code used a weird mix of yarn and npm and I wasn't sure if this was to support a particular build pipieline or something and didn't want to change it unnecessarily, but npm will work just fine - I'll ping you when it's updated.

Copy link
Owner

Choose a reason for hiding this comment

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

I was not aware of a mix between Yarn and NPM. That, must have been by mistake, my bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, my best guess is the (Yeoman?) template you originally used to create the extension was designed to work with yarn but worked just fine with npm as long as nobody tried to run the yarn-specific commands in the package.json scripts section.

It's all on npm now; merge whenever you're ready and I'll create the new PR with the typeChunksFromFile feature support. 👍🏼

Copy link
Owner

Choose a reason for hiding this comment

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

Yes I used Yeoman when I first created this project to get started. And as this was just a simple utility to help with presentations I never used any proper engineering principles. Just sort of hacked it together.

@mauricedb
Copy link
Owner

mauricedb commented Mar 22, 2024 via email

@dylanbeattie
Copy link
Contributor Author

Hey Maurice,

There are two reasons I plugged in WaitAfterNewLine as a special case. First, having used this for a week or so I didn't actually find any scenario or language where I didn't want that option, so having it enabled by default seemed like a good idea.

Second: the VS Code configuration editor for string arrays can't cope with literal newlines - if you put a \n in the JSON config, it shows as an empty string in the config editor, and if you enter a \n in the config editor UI it's persisted as \\n in the JSON.

I agree it would make more sense as just another entry in the array, but after playing around with it I decided the tooling made this more confusing than having an extra config setting - let me know what you think.

@mauricedb
Copy link
Owner

Hey Maurice,

There are two reasons I plugged in WaitAfterNewLine as a special case. First, having used this for a week or so I didn't actually find any scenario or language where I didn't want that option, so having it enabled by default seemed like a good idea.

Second: the VS Code configuration editor for string arrays can't cope with literal newlines - if you put a \n in the JSON config, it shows as an empty string in the config editor, and if you enter a \n in the config editor UI it's persisted as \\n in the JSON.

I agree it would make more sense as just another entry in the array, but after playing around with it I decided the tooling made this more confusing than having an extra config setting - let me know what you think.

👍 Right makes total sense.

Just leave it as is then and I will be happy to merge this when you create the second PR.

@mauricedb mauricedb merged commit 1c5111b into mauricedb:master Mar 24, 2024
1 check passed
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