-
Notifications
You must be signed in to change notification settings - Fork 621
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: make CI config DRY #470
Conversation
.ci/steps.win.yml
Outdated
@@ -0,0 +1,7 @@ | |||
steps: |
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'd refer ci
directory to .ci
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 thought about it, but won't it be confusing for users? It'll look like one of the available modules.
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.
ok good point
@ry this should be ready for review |
- bash: npm install eslint typescript@$(TS_VERSION) @typescript-eslint/eslint-plugin @typescript-eslint/parser eslint-config-prettier | ||
- powershell: iwr https://deno.land/x/install/install.ps1 -out install.ps1; .\install.ps1 $(DENO_VERSION) | ||
- bash: echo "##vso[task.prependpath]C:\Users\VssAdministrator\.deno\\bin" | ||
- bash: npx eslint **/*.ts --max-warnings=0 |
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.
Can the eslint be moved to common?
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.
Nope, it's run with npx
on Linux and Windows, but not on Mac. I tried running it with npx
under Mac as well, but there's not npx
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.
LGTM thanks!
Closes denoland#478 Related denoland#470
Changes: 1. Notifications use ULIDs for their IDs. 2. Removes the `notifications` index from KV, as it seems unnecessary. 3. Removed the `createdAt` property from notifications. 4. Removes `newNotificationProps()`, as it now seems unnecessary. Closes denoland#473 Towards denoland#470 CC @lino-levan (this might be a good reference to learn more about ULIDs)
Description This PR fixes denoland#470 Notes for Reviewers rename and restructure /tools folder --------- Signed-off-by: Jigar Rathod <[email protected]>
Reference
Using templates to configure CI to dry up configuration