-
Notifications
You must be signed in to change notification settings - Fork 11
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
Conversation
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.
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. |
So the actual feature I propose to add is implemented here: (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 😊 |
There was a problem hiding this 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.
src/test/index.ts
Outdated
// 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 👍🏼
There was a problem hiding this comment.
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.
Hi Dylan,
I was just looking at the *typeChunksFromFile* instruction you are
proposing and I like that way it works. Looks like that will make it much
easier to use with a large amount of input where you want additional
pauses, something I do quite often.
I would probably not have added the special case for the *waitAfterNewLine*
as it looks like it could just as easily be included in the
*waitAfterTyping* array but I don't have an issue with it either.
I would be delighted to include this PR.
Thank,
Maurice
Maurice de Beijer
ABL – The Problem Solver
Microsoft MVP
www.TheProblemSolver.nl
www.twitter.com/mauricedb
…On Thu, Mar 21, 2024 at 12:43 PM Dylan Beattie ***@***.***> wrote:
So the actual feature I propose to add is implemented here:
dylanbeattie#1 <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 😊
—
Reply to this email directly, view it on GitHub
<#18 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYMWIX7DRQI25O6MW7HWFDYZLBXTAVCNFSM6AAAAABE4ENNOWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJSGA2TAMBTGM>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
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 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. |
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!)